Bug 149340

Summary: Implement type conversion instructions in WebAssembly
Product: WebKit Reporter: Sukolsak Sakshuwong <sukolsak>
Component: JavaScriptCoreAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, commit-queue, fpizlo, ggaren, mark.lam, msaboff, saam, sukolsak, ysuzuki
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 146064    
Attachments:
Description Flags
Patch none

Description Sukolsak Sakshuwong 2015-09-18 03:07:19 PDT
Implement type conversion instructions in WebAssembly
Comment 1 Sukolsak Sakshuwong 2015-09-18 04:13:31 PDT
Created attachment 261498 [details]
Patch
Comment 2 WebKit Commit Bot 2015-09-18 04:16:32 PDT
Attachment 261498 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/wasm/WASMFunctionCompiler.h:1000:  JIT_OPERATION is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Mark Lam 2015-09-18 10:09:02 PDT
Comment on attachment 261498 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=261498&action=review

> Source/JavaScriptCore/wasm/WASMFunctionCompiler.h:378
> +    int buildConvertType(int, WASMExpressionType fromType, WASMExpressionType toType, WASMTypeConversion conversion)

Shouldn't the first argument and return type be of type ContextExpression?

> Source/JavaScriptCore/wasm/WASMFunctionCompiler.h:382
> +            load32(temporaryAddress(m_tempStackTop - 1), GPRInfo::regT0);

Everything looks fine, but I'm unfamiliar with this expression of "temporaryAddress(m_tempStackTop - 1)".  Just to make sure there is no bug here, can you explain what it means?  Specifically,
1. why is it called a "temporary" address?
2. why is it referring to "m_tempStackTop - 1"?  My interpretation is that you're operating on the operand at the top of the stack.  Hence, my naive expectation is that you would want "m_tempStackTop" instead?

Can you please clarify?  Thanks.
Comment 4 Sukolsak Sakshuwong 2015-09-18 13:37:15 PDT
(In reply to comment #3)
> Comment on attachment 261498 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=261498&action=review
> 
> > Source/JavaScriptCore/wasm/WASMFunctionCompiler.h:378
> > +    int buildConvertType(int, WASMExpressionType fromType, WASMExpressionType toType, WASMTypeConversion conversion)
> 
> Shouldn't the first argument and return type be of type ContextExpression?

ContextExpression is a macro for "typename Context::Expression". It is only used in the parser. Methods in the parser are templatized and take Context as the template parameter. The current possible Contexts are WASMFunctionSyntaxChecker WASMFunctionCompiler. In the future, we will have WASMFunctionLLVMIRGenerator.

In the context of WASMFunctionSyntaxChecker or WASMFunctionCompiler, Expression is just a typedef of int, because we never use it. In the context of WASMFunctionLLVMIRGenerator, Expression will be a typedef of FTL::LValue (or LLVMValueRef.)

I could use Expression, of course. But that seems to introduce unnecessary indirectness. I use the same design as the JSC parser. For example,

- In parser/SyntaxChecker.h, we have "int createBreakStatement(const JSTokenLocation&, int, int) { return StatementResult; }"

- In parser/ASTBuilder.h, we have "StatementNode* createBreakStatement(const JSTokenLocation& location, const Identifier* ident, const JSTextPosition& start, const JSTextPosition& end)"

> > Source/JavaScriptCore/wasm/WASMFunctionCompiler.h:382
> > +            load32(temporaryAddress(m_tempStackTop - 1), GPRInfo::regT0);
> 
> Everything looks fine, but I'm unfamiliar with this expression of
> "temporaryAddress(m_tempStackTop - 1)".  Just to make sure there is no bug
> here, can you explain what it means?  Specifically,
> 1. why is it called a "temporary" address?
> 2. why is it referring to "m_tempStackTop - 1"?  My interpretation is that
> you're operating on the operand at the top of the stack.  Hence, my naive
> expectation is that you would want "m_tempStackTop" instead?
> 
> Can you please clarify?  Thanks.

A WebAssembly call frame consists of [space for callee save registers] + [local variables] + [temporaries] + [padding for slow path calls] + [padding for stack pointer alignment]. We can think of [temporaries] as a stack, where m_tempStackTop is the size of the stack. m_tempStackTop starts from zero, because there is no value there. When there is a value there, temporaries[m_tempStackTop - 1] is the top value of the stack. temporaryAddress(m_tempStackTop - 1) is basically the address of temporaries[m_tempStackTop - 1].

We calculate the maximum size of [temporaries] in the syntax checking phrase.
Comment 5 Mark Lam 2015-09-18 13:39:21 PDT
Comment on attachment 261498 [details]
Patch

r=me
Comment 6 Sukolsak Sakshuwong 2015-09-18 13:42:05 PDT
(In reply to comment #4)
> temporaryAddress(m_tempStackTop - 1) is basically the address of
> temporaries[m_tempStackTop - 1].

The "temporaryAddress(m_tempStackTop - i)" idiom will go away when I implement peephole optimization. I will need to find where values are stored from the arguments and the return values.
Comment 7 WebKit Commit Bot 2015-09-18 14:30:12 PDT
Comment on attachment 261498 [details]
Patch

Clearing flags on attachment: 261498

Committed r189984: <http://trac.webkit.org/changeset/189984>
Comment 8 WebKit Commit Bot 2015-09-18 14:30:16 PDT
All reviewed patches have been landed.  Closing bug.