Bug 149340 - Implement type conversion instructions in WebAssembly
Summary: Implement type conversion instructions in WebAssembly
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 146064
  Show dependency treegraph
 
Reported: 2015-09-18 03:07 PDT by Sukolsak Sakshuwong
Modified: 2015-09-18 14:30 PDT (History)
9 users (show)

See Also:


Attachments
Patch (16.26 KB, patch)
2015-09-18 04:13 PDT, Sukolsak Sakshuwong
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.