[wasm] Adding feature to JIT a wasm function at runtime and hook up the compiled code into the indi…

[wasm] Adding feature to JIT a wasm function at runtime and hook up the compiled code into the indirect function table

The runtime JIT function is passed in the function table to hook up the compiled code and the starting address of the memory to locate the bytes to be compiled.

BUG=5044
Committed: https://crrev.com/de33e4bad271ec2f5cbfc0b040b15465b0a2d778
Cr-Commit-Position: refs/heads/master@{#37735}

[ritesht, 2016-07-12 00:04:56]

Description was changed from

==========
[wasm] Adding feature to JIT a wasm function at runtime and hook up the compiled
code into the indirect function table

The runtime JIT function is passed in the function table to hook up the compiled
code and the starting address of the memory to locate the bytes to be compiled.

Note: Currently, the code requires the user to input a list of all the local
declarations (inlcuding the parameters) along with the function body as a part
of the bytes that have to be JITed.
This could be fixed by inferring the parameter types from the signature and
storing the types in memory physically before the function body.

Fixed constructor declaration


Finished cleaning up dead code and other code formatting


removing some dead code


Changes made to sync with changes made in the master


Merge branch 'master' of https://chromium.googlesource.com/v8/v8 into jit_opcode


Added final test to JIT two functions Starting address of memory passed in to
the runtime function


Added test to JIT a single function


Merge branch 'master' of https://chromium.googlesource.com/v8/v8 into jit_opcode


Finished linking the compiled code from the wasm-function into the function
table.

WasmFunction objct has been created by passing into the run time, the
signature, index into the table, signature index, offset and size of the
bytes.

Fixed ruhtime JIT function with the appropriate argument checks


Merge branch 'master' of https://chromium.googlesource.com/v8/v8 into jit_opcode


Adding the signature index as a parameter


Merge branch 'master' of https://chromium.googlesource.com/v8/v8 into jit_opcode


Merge branch 'master' of https://chromium.googlesource.com/v7/v8 into jit_opcode


[wasm] Adding a feature to JIT wasm functions at runtime.


stuff


Add in flag.


Intermediate state

BUG=
==========

to

==========
[wasm] Adding feature to JIT a wasm function at runtime and hook up the compiled
code into the indirect function table

The runtime JIT function is passed in the function table to hook up the compiled
code and the starting address of the memory to locate the bytes to be compiled.

Note: Currently, the code requires the user to input a list of all the local
declarations (inlcuding the parameters) along with the function body as a part
of the bytes that have to be JITed.
This could be fixed by inferring the parameter types from the signature and
storing the types in memory physically before the function body.

BUG=5044

Fixed constructor declaration


Finished cleaning up dead code and other code formatting


removing some dead code


Changes made to sync with changes made in the master


Merge branch 'master' of https://chromium.googlesource.com/v8/v8 into jit_opcode


Added final test to JIT two functions Starting address of memory passed in to
the runtime function


Added test to JIT a single function


Merge branch 'master' of https://chromium.googlesource.com/v8/v8 into jit_opcode


Finished linking the compiled code from the wasm-function into the function
table.

WasmFunction objct has been created by passing into the run time, the
signature, index into the table, signature index, offset and size of the
bytes.

Fixed ruhtime JIT function with the appropriate argument checks


Merge branch 'master' of https://chromium.googlesource.com/v8/v8 into jit_opcode


Adding the signature index as a parameter


Merge branch 'master' of https://chromium.googlesource.com/v8/v8 into jit_opcode


Merge branch 'master' of https://chromium.googlesource.com/v7/v8 into jit_opcode


[wasm] Adding a feature to JIT wasm functions at runtime.


stuff


Add in flag.


Intermediate state
==========

[ritesht, 2016-07-12 00:05:10]

Description was changed from

==========
[wasm] Adding feature to JIT a wasm function at runtime and hook up the compiled
code into the indirect function table

The runtime JIT function is passed in the function table to hook up the compiled
code and the starting address of the memory to locate the bytes to be compiled.

Note: Currently, the code requires the user to input a list of all the local
declarations (inlcuding the parameters) along with the function body as a part
of the bytes that have to be JITed.
This could be fixed by inferring the parameter types from the signature and
storing the types in memory physically before the function body.

BUG=5044

Fixed constructor declaration


Finished cleaning up dead code and other code formatting


removing some dead code


Changes made to sync with changes made in the master


Merge branch 'master' of https://chromium.googlesource.com/v8/v8 into jit_opcode


Added final test to JIT two functions Starting address of memory passed in to
the runtime function


Added test to JIT a single function


Merge branch 'master' of https://chromium.googlesource.com/v8/v8 into jit_opcode


Finished linking the compiled code from the wasm-function into the function
table.

WasmFunction objct has been created by passing into the run time, the
signature, index into the table, signature index, offset and size of the
bytes.

Fixed ruhtime JIT function with the appropriate argument checks


Merge branch 'master' of https://chromium.googlesource.com/v8/v8 into jit_opcode


Adding the signature index as a parameter


Merge branch 'master' of https://chromium.googlesource.com/v8/v8 into jit_opcode


Merge branch 'master' of https://chromium.googlesource.com/v7/v8 into jit_opcode


[wasm] Adding a feature to JIT wasm functions at runtime.


stuff


Add in flag.


Intermediate state
==========

to

==========
[wasm] Adding feature to JIT a wasm function at runtime and hook up the compiled
code into the indirect function table

The runtime JIT function is passed in the function table to hook up the compiled
code and the starting address of the memory to locate the bytes to be compiled.

Note: Currently, the code requires the user to input a list of all the local
declarations (inlcuding the parameters) along with the function body as a part
of the bytes that have to be JITed.
This could be fixed by inferring the parameter types from the signature and
storing the types in memory physically before the function body.

BUG=5044
==========

[ritesht, 2016-07-12 00:06:48]

ritesht@google.com changed reviewers:
+ bradnelson@chromium.org, mtrofin@chromium.org

[ritesht, 2016-07-12 00:11:06]

ritesht@google.com changed reviewers:
- bradnelson@chromium.org, mtrofin@chromium.org

[ritesht, 2016-07-12 01:50:15]

ritesht@google.com changed reviewers:
+ bradnelson@chromium.org, mtrofin@chromium.org

[Mircea Trofin, 2016-07-12 02:48:59]

On 2016/07/12 01:50:27, ritesht wrote:

I'm not sure I understand the scope of the feature. Is JIT-ing a single function
intentionally scoped to "leaf" functions only, i.e. functions that don't call
back into the module (and/or imports)? Once inserted at an index, can I JIT
another function and insert it at the same index - if yes, have we thought
through the implications this will have on multi-threading?

[Mircea Trofin, 2016-07-12 02:49:13]

https://codereview.chromium.org/2137993003/diff/80001/src/compiler/wasm-compi...
File src/compiler/wasm-compiler.cc (right):

https://codereview.chromium.org/2137993003/diff/80001/src/compiler/wasm-compi...
src/compiler/wasm-compiler.cc:2102: uint32_t sig_index,
looks like base, length, index, and sig are all not mutated in this API. Could
you mark them as const?

https://codereview.chromium.org/2137993003/diff/80001/src/compiler/wasm-compi...
src/compiler/wasm-compiler.cc:2112: uint32_t inputs_size = 13 + return_count +
parameter_count;
what's 13? (is there a constant or should you define perhaps a constant)?

https://codereview.chromium.org/2137993003/diff/80001/src/compiler/wasm-compi...
src/compiler/wasm-compiler.cc:2114: inputs[0] =
jsgraph()->CEntryStubConstant(fun->result_size);  // C entry
remove comment ("// C entry"), doesn't add info.

https://codereview.chromium.org/2137993003/diff/80001/src/compiler/wasm-compi...
src/compiler/wasm-compiler.cc:2125: for (uint32_t i = 0; i < return_count; i++)
{
++i

https://codereview.chromium.org/2137993003/diff/80001/src/compiler/wasm-compi...
src/compiler/wasm-compiler.cc:2130: for (uint32_t i = 0; i < parameter_count;
i++) {
++i

https://codereview.chromium.org/2137993003/diff/80001/src/compiler/wasm-compi...
src/compiler/wasm-compiler.cc:2135: const uint32_t args_offset = 8 +
return_count + parameter_count;
could you define constants for all these indices (e.g. the "1" in inputs[1], "8"
in args_offset = 8 + ...), for 
maintainability

https://codereview.chromium.org/2137993003/diff/80001/src/compiler/wasm-compi...
src/compiler/wasm-compiler.cc:2479: Node*
WasmGraphBuilder::BuildChangeIntPtrToSmi(Node* value) {
"value" is const?

https://codereview.chromium.org/2137993003/diff/80001/src/runtime/runtime-int...
File src/runtime/runtime-internal.cc (right):

https://codereview.chromium.org/2137993003/diff/80001/src/runtime/runtime-int...
src/runtime/runtime-internal.cc:158: DCHECK_LE(7, args.length());
same comment as before about numbers - if possible, define and use constants.
Helps maintainability.

https://codereview.chromium.org/2137993003/diff/80001/src/runtime/runtime-int...
src/runtime/runtime-internal.cc:201: function_table->set(index +
function_table->length() / 2, *code);
do you want to check that index < function_table->length() / 2?

https://codereview.chromium.org/2137993003/diff/80001/src/runtime/runtime-uti...
File src/runtime/runtime-utils.h (right):

https://codereview.chromium.org/2137993003/diff/80001/src/runtime/runtime-uti...
src/runtime/runtime-utils.h:53: #define CONVERT_INTPTR_ARG_CHECKED(name, index)
\
Is the comment above still pertinent?

https://codereview.chromium.org/2137993003/diff/80001/src/runtime/runtime.h
File src/runtime/runtime.h (right):

https://codereview.chromium.org/2137993003/diff/80001/src/runtime/runtime.h#n...
src/runtime/runtime.h:300: F(JITSingleFunction, -1, 1)                       \
Mind adding a comment on top of this file describing what a negative number of
arguments means?

https://codereview.chromium.org/2137993003/diff/80001/src/wasm/ast-decoder.cc
File src/wasm/ast-decoder.cc (right):

https://codereview.chromium.org/2137993003/diff/80001/src/wasm/ast-decoder.cc...
src/wasm/ast-decoder.cc:196: inline bool Complete(const byte* pc,
JITSingleFunctionOperand& operand) {
Coding convention says pass-by-reference parameters should be const, or you pass
a pointer.
https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#...

https://codereview.chromium.org/2137993003/diff/80001/src/wasm/ast-decoder.cc...
src/wasm/ast-decoder.cc:206: if (Complete(pc, operand)) {
same as above

https://codereview.chromium.org/2137993003/diff/80001/test/mjsunit/wasm/jit-s...
File test/mjsunit/wasm/jit-single-function.js (right):

https://codereview.chromium.org/2137993003/diff/80001/test/mjsunit/wasm/jit-s...
test/mjsunit/wasm/jit-single-function.js:5: // Flags: --wasm-jit-prototype
why flip these switches?

[John, 2016-07-13 19:56:58]

jpp@chromium.org changed reviewers:
+ jpp@chromium.org

[John, 2016-07-13 19:56:59]

all optional

https://codereview.chromium.org/2137993003/diff/80001/src/compiler/wasm-compi...
File src/compiler/wasm-compiler.cc (right):

https://codereview.chromium.org/2137993003/diff/80001/src/compiler/wasm-compi...
src/compiler/wasm-compiler.cc:2102: uint32_t sig_index,
On 2016/07/12 02:49:12, Mircea Trofin wrote:
> looks like base, length, index, and sig are all not mutated in this API. Could
> you mark them as const?

Const-correctness is good, but it seems this module does not use const for
parameters. Maybe, for consistency, we should let it as is? (of course, then the
lack of const qualifiers become a self-fulfilling prophecy, and nobody will ever
bother to fix...)

https://codereview.chromium.org/2137993003/diff/80001/src/compiler/wasm-compi...
src/compiler/wasm-compiler.cc:2145: CallDescriptor* desc =
Linkage::GetRuntimeCallDescriptor(
I would probably use auto* here, but auto isn't used anywhere else...

https://codereview.chromium.org/2137993003/diff/80001/src/wasm/ast-decoder.h
File src/wasm/ast-decoder.h (right):

https://codereview.chromium.org/2137993003/diff/80001/src/wasm/ast-decoder.h#...
src/wasm/ast-decoder.h:153: struct JITSingleFunctionOperand {
why not make this a class, with getters/setters?

https://codereview.chromium.org/2137993003/diff/80001/src/wasm/wasm-module.h
File src/wasm/wasm-module.h (right):

https://codereview.chromium.org/2137993003/diff/80001/src/wasm/wasm-module.h#...
src/wasm/wasm-module.h:190: explicit WasmModule(byte* module_start = nullptr);
optional: It probably does not matter, but what do you think of using delagate
constructors here, in lieu of default argument value:

WasmModule() : WasmModule(nullptr) {}
explicit WasmModule(byte* module_start);

https://codereview.chromium.org/2137993003/diff/80001/test/mjsunit/wasm/jit-s...
File test/mjsunit/wasm/jit-single-function.js (right):

https://codereview.chromium.org/2137993003/diff/80001/test/mjsunit/wasm/jit-s...
test/mjsunit/wasm/jit-single-function.js:14: var kSig_i_iiiii =
makeSig([kAstI32, kAstI32, kAstI32, kAstI32, kAstI32], [kAstI32]);
this line is too long.

[ritesht, 2016-07-13 23:58:45]

https://codereview.chromium.org/2137993003/diff/80001/src/compiler/wasm-compi...
File src/compiler/wasm-compiler.cc (right):

https://codereview.chromium.org/2137993003/diff/80001/src/compiler/wasm-compi...
src/compiler/wasm-compiler.cc:2102: uint32_t sig_index,
On 2016/07/12 02:49:12, Mircea Trofin wrote:
> looks like base, length, index, and sig are all not mutated in this API. Could
> you mark them as const?

Done.

https://codereview.chromium.org/2137993003/diff/80001/src/compiler/wasm-compi...
src/compiler/wasm-compiler.cc:2102: uint32_t sig_index,
On 2016/07/13 19:56:59, John wrote:
> On 2016/07/12 02:49:12, Mircea Trofin wrote:
> > looks like base, length, index, and sig are all not mutated in this API.
Could
> > you mark them as const?
> 
> Const-correctness is good, but it seems this module does not use const for
> parameters. Maybe, for consistency, we should let it as is? (of course, then
the
> lack of const qualifiers become a self-fulfilling prophecy, and nobody will
ever
> bother to fix...)

Acknowledged.

https://codereview.chromium.org/2137993003/diff/80001/src/compiler/wasm-compi...
src/compiler/wasm-compiler.cc:2112: uint32_t inputs_size = 13 + return_count +
parameter_count;
On 2016/07/12 02:49:12, Mircea Trofin wrote:
> what's 13? (is there a constant or should you define perhaps a constant)?

Done.

https://codereview.chromium.org/2137993003/diff/80001/src/compiler/wasm-compi...
src/compiler/wasm-compiler.cc:2114: inputs[0] =
jsgraph()->CEntryStubConstant(fun->result_size);  // C entry
On 2016/07/12 02:49:12, Mircea Trofin wrote:
> remove comment ("// C entry"), doesn't add info.

Done.

https://codereview.chromium.org/2137993003/diff/80001/src/compiler/wasm-compi...
src/compiler/wasm-compiler.cc:2125: for (uint32_t i = 0; i < return_count; i++)
{
On 2016/07/12 02:49:12, Mircea Trofin wrote:
> ++i

Done.

https://codereview.chromium.org/2137993003/diff/80001/src/compiler/wasm-compi...
src/compiler/wasm-compiler.cc:2130: for (uint32_t i = 0; i < parameter_count;
i++) {
On 2016/07/12 02:49:12, Mircea Trofin wrote:
> ++i

Done.

https://codereview.chromium.org/2137993003/diff/80001/src/compiler/wasm-compi...
src/compiler/wasm-compiler.cc:2135: const uint32_t args_offset = 8 +
return_count + parameter_count;
On 2016/07/12 02:49:12, Mircea Trofin wrote:
> could you define constants for all these indices (e.g. the "1" in inputs[1],
"8"
> in args_offset = 8 + ...), for 
> maintainability

Done.

https://codereview.chromium.org/2137993003/diff/80001/src/compiler/wasm-compi...
src/compiler/wasm-compiler.cc:2145: CallDescriptor* desc =
Linkage::GetRuntimeCallDescriptor(
On 2016/07/13 19:56:59, John wrote:
> I would probably use auto* here, but auto isn't used anywhere else...

Acknowledged.

https://codereview.chromium.org/2137993003/diff/80001/src/compiler/wasm-compi...
src/compiler/wasm-compiler.cc:2479: Node*
WasmGraphBuilder::BuildChangeIntPtrToSmi(Node* value) {
On 2016/07/12 02:49:12, Mircea Trofin wrote:
> "value" is const?

Removed function. No longer needed.

https://codereview.chromium.org/2137993003/diff/80001/src/runtime/runtime-int...
File src/runtime/runtime-internal.cc (right):

https://codereview.chromium.org/2137993003/diff/80001/src/runtime/runtime-int...
src/runtime/runtime-internal.cc:158: DCHECK_LE(7, args.length());
On 2016/07/12 02:49:12, Mircea Trofin wrote:
> same comment as before about numbers - if possible, define and use constants.
> Helps maintainability.

Done.

https://codereview.chromium.org/2137993003/diff/80001/src/runtime/runtime-int...
src/runtime/runtime-internal.cc:201: function_table->set(index +
function_table->length() / 2, *code);
On 2016/07/12 02:49:13, Mircea Trofin wrote:
> do you want to check that index < function_table->length() / 2?

Done.

https://codereview.chromium.org/2137993003/diff/80001/src/runtime/runtime.h
File src/runtime/runtime.h (right):

https://codereview.chromium.org/2137993003/diff/80001/src/runtime/runtime.h#n...
src/runtime/runtime.h:300: F(JITSingleFunction, -1, 1)                       \
On 2016/07/12 02:49:13, Mircea Trofin wrote:
> Mind adding a comment on top of this file describing what a negative number of
> arguments means?

Done.

https://codereview.chromium.org/2137993003/diff/80001/src/wasm/ast-decoder.cc
File src/wasm/ast-decoder.cc (right):

https://codereview.chromium.org/2137993003/diff/80001/src/wasm/ast-decoder.cc...
src/wasm/ast-decoder.cc:196: inline bool Complete(const byte* pc,
JITSingleFunctionOperand& operand) {
On 2016/07/12 02:49:13, Mircea Trofin wrote:
> Coding convention says pass-by-reference parameters should be const, or you
pass
> a pointer.
>
https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#...

The other functions don't seem to use this. Maybe we can change the others in
another cl?

https://codereview.chromium.org/2137993003/diff/80001/src/wasm/ast-decoder.cc...
src/wasm/ast-decoder.cc:206: if (Complete(pc, operand)) {
On 2016/07/12 02:49:13, Mircea Trofin wrote:
> same as above

Acknowledged.

https://codereview.chromium.org/2137993003/diff/80001/src/wasm/ast-decoder.h
File src/wasm/ast-decoder.h (right):

https://codereview.chromium.org/2137993003/diff/80001/src/wasm/ast-decoder.h#...
src/wasm/ast-decoder.h:153: struct JITSingleFunctionOperand {
On 2016/07/13 19:56:59, John wrote:
> why not make this a class, with getters/setters?

It looks like the rest of the module doesn't use classes. I'll keep this for
consistency now.

https://codereview.chromium.org/2137993003/diff/80001/src/wasm/wasm-module.h
File src/wasm/wasm-module.h (right):

https://codereview.chromium.org/2137993003/diff/80001/src/wasm/wasm-module.h#...
src/wasm/wasm-module.h:190: explicit WasmModule(byte* module_start = nullptr);
On 2016/07/13 19:56:59, John wrote:
> optional: It probably does not matter, but what do you think of using delagate
> constructors here, in lieu of default argument value:
> 
> WasmModule() : WasmModule(nullptr) {}
> explicit WasmModule(byte* module_start);

Done.

https://codereview.chromium.org/2137993003/diff/80001/test/mjsunit/wasm/jit-s...
File test/mjsunit/wasm/jit-single-function.js (right):

https://codereview.chromium.org/2137993003/diff/80001/test/mjsunit/wasm/jit-s...
test/mjsunit/wasm/jit-single-function.js:5: // Flags: --wasm-jit-prototype
On 2016/07/12 02:49:13, Mircea Trofin wrote:
> why flip these switches?

This basically enables the flag to gate the experimental wasm code in.

https://codereview.chromium.org/2137993003/diff/80001/test/mjsunit/wasm/jit-s...
test/mjsunit/wasm/jit-single-function.js:14: var kSig_i_iiiii =
makeSig([kAstI32, kAstI32, kAstI32, kAstI32, kAstI32], [kAstI32]);
On 2016/07/13 19:56:59, John wrote:
> this line is too long.

Done.

[ritesht, 2016-07-13 23:59:40]

Description was changed from

==========
[wasm] Adding feature to JIT a wasm function at runtime and hook up the compiled
code into the indirect function table

The runtime JIT function is passed in the function table to hook up the compiled
code and the starting address of the memory to locate the bytes to be compiled.

Note: Currently, the code requires the user to input a list of all the local
declarations (inlcuding the parameters) along with the function body as a part
of the bytes that have to be JITed.
This could be fixed by inferring the parameter types from the signature and
storing the types in memory physically before the function body.

BUG=5044
==========

to

==========
[wasm] Adding feature to JIT a wasm function at runtime and hook up the compiled
code into the indirect function table

The runtime JIT function is passed in the function table to hook up the compiled
code and the starting address of the memory to locate the bytes to be compiled.

BUG=5044
==========

[ritesht, 2016-07-14 00:00:57]

On 2016/07/12 02:48:59, Mircea Trofin wrote:
> On 2016/07/12 01:50:27, ritesht wrote:
> 
> I'm not sure I understand the scope of the feature. Is JIT-ing a single
function
> intentionally scoped to "leaf" functions only, i.e. functions that don't call
> back into the module (and/or imports)? Once inserted at an index, can I JIT
> another function and insert it at the same index - if yes, have we thought
> through the implications this will have on multi-threading?

This feature is mostly experimental for now. This feature doesn't yet address
the issue of inserting two JITed wasm functions at the same index.

[Mircea Trofin, 2016-07-14 00:15:59]

On 2016/07/14 00:00:57, ritesht wrote:
> On 2016/07/12 02:48:59, Mircea Trofin wrote:
> > On 2016/07/12 01:50:27, ritesht wrote:
> > 
> > I'm not sure I understand the scope of the feature. Is JIT-ing a single
> function
> > intentionally scoped to "leaf" functions only, i.e. functions that don't
call
> > back into the module (and/or imports)? Once inserted at an index, can I JIT
> > another function and insert it at the same index - if yes, have we thought
> > through the implications this will have on multi-threading?
> 
> This feature is mostly experimental for now. This feature doesn't yet address
> the issue of inserting two JITed wasm functions at the same index.

lgtm, one nit

[Mircea Trofin, 2016-07-14 00:16:15]

lgtm, one nit

https://codereview.chromium.org/2137993003/diff/80001/test/mjsunit/wasm/jit-s...
File test/mjsunit/wasm/jit-single-function.js (right):

https://codereview.chromium.org/2137993003/diff/80001/test/mjsunit/wasm/jit-s...
test/mjsunit/wasm/jit-single-function.js:5: // Flags: --wasm-jit-prototype
On 2016/07/13 23:58:45, ritesht wrote:
> On 2016/07/12 02:49:13, Mircea Trofin wrote:
> > why flip these switches?
> 
> This basically enables the flag to gate the experimental wasm code in.

Sorry, I meant, why is wasm-jit-prototype on top now. It used to be under
--expose-wasm.

[ritesht, 2016-07-14 00:18:38]

https://codereview.chromium.org/2137993003/diff/80001/test/mjsunit/wasm/jit-s...
File test/mjsunit/wasm/jit-single-function.js (right):

https://codereview.chromium.org/2137993003/diff/80001/test/mjsunit/wasm/jit-s...
test/mjsunit/wasm/jit-single-function.js:5: // Flags: --wasm-jit-prototype
On 2016/07/14 00:16:15, Mircea Trofin wrote:
> On 2016/07/13 23:58:45, ritesht wrote:
> > On 2016/07/12 02:49:13, Mircea Trofin wrote:
> > > why flip these switches?
> > 
> > This basically enables the flag to gate the experimental wasm code in.
> 
> Sorry, I meant, why is wasm-jit-prototype on top now. It used to be under
> --expose-wasm.

Ok. Fixed.

[ritesht, 2016-07-14 01:10:30]

The CQ bit was checked by ritesht@google.com

[ritesht, 2016-07-14 01:10:30]

The patchset sent to the CQ was uploaded after l-g-t-m from mtrofin@chromium.org
Link to the patchset: https://codereview.chromium.org/2137993003/#ps140001
(title: "Fixing unit test-")

[commit-bot: I haz the power, 2016-07-14 01:10:35]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-14 01:12:51]

Description was changed from

==========
[wasm] Adding feature to JIT a wasm function at runtime and hook up the compiled
code into the indirect function table

The runtime JIT function is passed in the function table to hook up the compiled
code and the starting address of the memory to locate the bytes to be compiled.

BUG=5044
==========

to

==========
[wasm] Adding feature to JIT a wasm function at runtime and hook up the compiled
code into the indirect function table

The runtime JIT function is passed in the function table to hook up the compiled
code and the starting address of the memory to locate the bytes to be compiled.

BUG=5044
==========

[commit-bot: I haz the power, 2016-07-14 01:12:52]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-07-14 01:13:57]

Description was changed from

==========
[wasm] Adding feature to JIT a wasm function at runtime and hook up the compiled
code into the indirect function table

The runtime JIT function is passed in the function table to hook up the compiled
code and the starting address of the memory to locate the bytes to be compiled.

BUG=5044
==========

to

==========
[wasm] Adding feature to JIT a wasm function at runtime and hook up the compiled
code into the indirect function table

The runtime JIT function is passed in the function table to hook up the compiled
code and the starting address of the memory to locate the bytes to be compiled.

BUG=5044

Committed: https://crrev.com/de33e4bad271ec2f5cbfc0b040b15465b0a2d778
Cr-Commit-Position: refs/heads/master@{#37735}
==========

[commit-bot: I haz the power, 2016-07-14 01:13:58]

Patchset 8 (id:??) landed as
https://crrev.com/de33e4bad271ec2f5cbfc0b040b15465b0a2d778
Cr-Commit-Position: refs/heads/master@{#37735}

[titzer, 2016-07-14 08:59:43]

titzer@chromium.org changed reviewers:
+ titzer@chromium.org

[titzer, 2016-07-14 08:59:44]

https://codereview.chromium.org/2137993003/diff/140001/src/compiler/wasm-comp...
File src/compiler/wasm-compiler.cc (right):

https://codereview.chromium.org/2137993003/diff/140001/src/compiler/wasm-comp...
src/compiler/wasm-compiler.cc:2101: Node*
WasmGraphBuilder::JITSingleFunction(Node* const base, Node* const length,
This should have been named BuildXXX, since it builds the graph that does
JITing, it itself doesn't do JITing.

https://codereview.chromium.org/2137993003/diff/140001/src/messages.h
File src/messages.h (right):

https://codereview.chromium.org/2137993003/diff/140001/src/messages.h#newcode496
src/messages.h:496: T(WasmTrapInvalidIndex, "invalid index into function table")
This is the same condition as TrapFuncInvalid.

https://codereview.chromium.org/2137993003/diff/140001/src/runtime/runtime-wa...
File src/runtime/runtime-wasm.cc (right):

https://codereview.chromium.org/2137993003/diff/140001/src/runtime/runtime-wa...
src/runtime/runtime-wasm.cc:118: const int fixed_args = 6;
Most of this functionality should somehow be in src/wasm, since it uses lots of
internal details.

https://codereview.chromium.org/2137993003/diff/140001/test/mjsunit/wasm/jit-...
File test/mjsunit/wasm/jit-single-function.js (right):

https://codereview.chromium.org/2137993003/diff/140001/test/mjsunit/wasm/jit-...
test/mjsunit/wasm/jit-single-function.js:11: var module = (function () {
There should be additional cctests for this functionality, to keep the JS API
out of the system under test.

[ritesht, 2016-07-14 18:10:00]

https://codereview.chromium.org/2137993003/diff/140001/src/compiler/wasm-comp...
File src/compiler/wasm-compiler.cc (right):

https://codereview.chromium.org/2137993003/diff/140001/src/compiler/wasm-comp...
src/compiler/wasm-compiler.cc:2101: Node*
WasmGraphBuilder::JITSingleFunction(Node* const base, Node* const length,
On 2016/07/14 08:59:43, titzer wrote:
> This should have been named BuildXXX, since it builds the graph that does
> JITing, it itself doesn't do JITing.

I think in this module, they don't seem to use "Build" as the name for anything.
'CallIndirect' also builds a graph but doesn't do the indirect call. Should I
keep this for the sake of consistency or change it?

https://codereview.chromium.org/2137993003/diff/140001/src/messages.h
File src/messages.h (right):

https://codereview.chromium.org/2137993003/diff/140001/src/messages.h#newcode496
src/messages.h:496: T(WasmTrapInvalidIndex, "invalid index into function table")
On 2016/07/14 08:59:43, titzer wrote:
> This is the same condition as TrapFuncInvalid.

The point of this is to draw the distinction between calling a function that is
already hooked to the table and a slot in the table where a potential compiled
code would be hooked to. I thought this would be more appropriate as the
function doesn't exist yet at that particular index.

https://codereview.chromium.org/2137993003/diff/140001/src/runtime/runtime-wa...
File src/runtime/runtime-wasm.cc (right):

https://codereview.chromium.org/2137993003/diff/140001/src/runtime/runtime-wa...
src/runtime/runtime-wasm.cc:118: const int fixed_args = 6;
On 2016/07/14 08:59:43, titzer wrote:
> Most of this functionality should somehow be in src/wasm, since it uses lots
of
> internal details.

Acknowledged.

https://codereview.chromium.org/2137993003/diff/140001/test/mjsunit/wasm/jit-...
File test/mjsunit/wasm/jit-single-function.js (right):

https://codereview.chromium.org/2137993003/diff/140001/test/mjsunit/wasm/jit-...
test/mjsunit/wasm/jit-single-function.js:11: var module = (function () {
On 2016/07/14 08:59:44, titzer wrote:
> There should be additional cctests for this functionality, to keep the JS API
> out of the system under test.

Acknowledged.