Bug 149340 – Implement type conversion instructions in WebAssembly

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Sukolsak Sakshuwong]

Implement type conversion instructions in WebAssembly

[Sukolsak Sakshuwong]

Created attachment 261498 [details]
Patch

[WebKit Commit Bot]

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.

[Mark Lam]

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.

[Sukolsak Sakshuwong]

(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.

[Mark Lam]

Comment on attachment 261498 [details]
Patch

r=me

[Sukolsak Sakshuwong]

(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.

[WebKit Commit Bot]

Comment on attachment 261498 [details]
Patch

Clearing flags on attachment: 261498

Committed r189984: <http://trac.webkit.org/changeset/189984>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.