Hooking up asm-wasm conversion.

Hooking up asm-wasm conversion.

Directs 'use asm' traffic through asm-wasm conversion when --validate-asm is passed.

Adds a builtin that handles the fallback to JS.

BUG= https://bugs.chromium.org/p/v8/issues/detail?id=4203
TEST=asm-wasm
R=mstarzinger@chromium.org,titzer@chromium.org
LOG=N

Committed: https://crrev.com/f20323dce2a796f25eddec83c34b97127d046745
Cr-Commit-Position: refs/heads/master@{#37470}

[bradnelson, 2016-06-13 06:36:39]

Description was changed from

==========
Hooking up asm-wasm conversion.

BUG= https://bugs.chromium.org/p/v8/issues/detail?id=4203
TEST=asm-wasm
R=titzer@chromium.org,aseemgarg@chromium.org
LOG=N
==========

to

==========
Hooking up asm-wasm conversion.

BUG= https://bugs.chromium.org/p/v8/issues/detail?id=4203
TEST=asm-wasm
R=mstarzinger@chromium.org,titzer@chromium.org
LOG=N
==========

[bradnelson, 2016-06-13 06:36:39]

bradnelson@chromium.org changed reviewers:
+ mstarzinger@chromium.org
- aseemgarg@chromium.org

[bradnelson, 2016-06-13 06:37:40]

Description was changed from

==========
Hooking up asm-wasm conversion.

BUG= https://bugs.chromium.org/p/v8/issues/detail?id=4203
TEST=asm-wasm
R=mstarzinger@chromium.org,titzer@chromium.org
LOG=N
==========

to

==========
Hooking up asm-wasm conversion.

Directs 'use asm' traffic through asm-wasm conversion when --validate-asm is
passed.

BUG= https://bugs.chromium.org/p/v8/issues/detail?id=4203
TEST=asm-wasm
R=mstarzinger@chromium.org,titzer@chromium.org
LOG=N
==========

[bradnelson, 2016-06-13 08:52:20]

Michael, Ben, is this approach sane?

Any guesses why I get the stack trace below for one of the
mjsunit/asm/infinite-loops.js, others basically working.

Thanks!

 2: v8::internal::Map::GetConstructorFunctionIndex()
 3: v8::internal::Object::GetRootMap(v8::internal::Isolate*)
 4:
v8::internal::LookupIterator::GetRootForNonJSReceiver(v8::internal::Isolate*,
v8::internal::Handle<v8::internal::Object>, unsigned int)
 5: v8::internal::LookupIterator::GetRoot(v8::internal::Isolate*,
v8::internal::Handle<v8::internal::Object>, unsigned int)
 6:
v8::internal::LookupIterator::LookupIterator(v8::internal::Handle<v8::internal::Object>,
v8::internal::Handle<v8::internal::Name>,
v8::internal::LookupIterator::Configuration)
 7: v8::internal::LoadIC::Load(v8::internal::Handle<v8::internal::Object>,
v8::internal::Handle<v8::internal::Name>)
 8: 0x129cae9
 9: v8::internal::Runtime_LoadIC_Miss(int, v8::internal::Object**,
v8::internal::Isolate*)

[Michael Starzinger, 2016-06-13 14:10:44]

To answer your question: My first guess would be that a code object escaped into
the JavaScript world and is then subsequently being operated on (i.e. loaded
from via LoadIC). I think this is related to one of my comments. You can verify
my theory by checking whether the "receiver" in Runtime_LoadIC_Miss is a code
object.

https://codereview.chromium.org/2057403003/diff/80001/src/builtins.cc
File src/builtins.cc (right):

https://codereview.chromium.org/2057403003/diff/80001/src/builtins.cc#newcode...
src/builtins.cc:5355: BUILTIN(CompileAsmJs) {
nit: Could we call this InstantiateAsmJs (or similar) instead? Because IIUC this
is the actual module instntiation code. It is not really triggering compilation.

https://codereview.chromium.org/2057403003/diff/80001/src/builtins.cc#newcode...
src/builtins.cc:5380: return function->code();
This will return the compiled code to JavaScript. IIUC we should tail-call into
this code so that it can instantiate the asm.js module and return the module
instead. I am not sure this can be expressed in C++ code. I think this needs to
be implemented as an assembly builtin (preferably using the TurboFan
CodeStubAssembler).

https://codereview.chromium.org/2057403003/diff/80001/src/objects-debug.cc
File src/objects-debug.cc (right):

https://codereview.chromium.org/2057403003/diff/80001/src/objects-debug.cc#ne...
src/objects-debug.cc:576: HasBuiltinFunctionId() || HasBytecodeArray() ||
HasAsmWasmData());
nit: I know this is not part of your change, but the check for
"HasBuiltinFunctionId" is obsolete by now. We could drop it from this
disjunction?

[bradnelson, 2016-06-27 05:58:00]

Description was changed from

==========
Hooking up asm-wasm conversion.

Directs 'use asm' traffic through asm-wasm conversion when --validate-asm is
passed.

BUG= https://bugs.chromium.org/p/v8/issues/detail?id=4203
TEST=asm-wasm
R=mstarzinger@chromium.org,titzer@chromium.org
LOG=N
==========

to

==========
Hooking up asm-wasm conversion.

Directs 'use asm' traffic through asm-wasm conversion when --validate-asm is
passed.

Adds a builtin that handles the fallback to JS only for x64 for now.

BUG= https://bugs.chromium.org/p/v8/issues/detail?id=4203
TEST=asm-wasm
R=mstarzinger@chromium.org,titzer@chromium.org
LOG=N
==========

[bradnelson, 2016-06-27 07:56:10]

PTAL

https://codereview.chromium.org/2057403003/diff/80001/src/builtins.cc
File src/builtins.cc (right):

https://codereview.chromium.org/2057403003/diff/80001/src/builtins.cc#newcode...
src/builtins.cc:5355: BUILTIN(CompileAsmJs) {
On 2016/06/13 14:10:43, Michael Starzinger wrote:
> nit: Could we call this InstantiateAsmJs (or similar) instead? Because IIUC
this
> is the actual module instntiation code. It is not really triggering
compilation.

Done.

https://codereview.chromium.org/2057403003/diff/80001/src/builtins.cc#newcode...
src/builtins.cc:5380: return function->code();
On 2016/06/13 14:10:43, Michael Starzinger wrote:
> This will return the compiled code to JavaScript. IIUC we should tail-call
into
> this code so that it can instantiate the asm.js module and return the module
> instead. I am not sure this can be expressed in C++ code. I think this needs
to
> be implemented as an assembly builtin (preferably using the TurboFan
> CodeStubAssembler).

Struggled with this a bit.

I don't believe it can be expressed with the code stub assembler currently. (I
might be game to add it, if you think supporting a non-local goto is feasible?)

It requires being able to speculatively call a function, then return swallowing
a frame on success, or tail call to generated code. Both of which are currently
per arch.

Eager to hear if you've got better ideas :-)

https://codereview.chromium.org/2057403003/diff/80001/src/objects-debug.cc
File src/objects-debug.cc (right):

https://codereview.chromium.org/2057403003/diff/80001/src/objects-debug.cc#ne...
src/objects-debug.cc:576: HasBuiltinFunctionId() || HasBytecodeArray() ||
HasAsmWasmData());
On 2016/06/13 14:10:43, Michael Starzinger wrote:
> nit: I know this is not part of your change, but the check for
> "HasBuiltinFunctionId" is obsolete by now. We could drop it from this
> disjunction?

Has gone away since.

[bradn, 2016-06-28 16:03:29]

bradnelson@google.com changed reviewers:
+ bradnelson@google.com

[bradn, 2016-06-28 16:03:30]

Ping?

[Michael Starzinger, 2016-06-29 08:29:23]

Approach looks good. Just a couple of minor comments. Lets do the ports to other
architectures as part of this CL. Usually we do ports to ia32, x64, ARM, ARM64
(and sometimes MIPS) ourselves. Handling of ports is documented here:

https://github.com/v8/v8/wiki/Handling-of-Ports

https://codereview.chromium.org/2057403003/diff/180001/src/builtins.cc
File src/builtins.cc (right):

https://codereview.chromium.org/2057403003/diff/180001/src/builtins.cc#newcode30
src/builtins.cc:30: #include "src/wasm/wasm-js.h"
nit: Include shouldn't be needed anymore. Let's drop it.

https://codereview.chromium.org/2057403003/diff/180001/src/builtins.h
File src/builtins.h (right):

https://codereview.chromium.org/2057403003/diff/180001/src/builtins.h#newcode181
src/builtins.h:181: #if defined(V8_TARGET_ARCH_X64)
We usually implement new features on all architectures in one go. Lets drop
these #ifdef guards.

https://codereview.chromium.org/2057403003/diff/180001/src/builtins.h#newcode481
src/builtins.h:481: #if defined(V8_TARGET_ARCH_X64)
Likewise.

https://codereview.chromium.org/2057403003/diff/180001/src/builtins.h#newcode483
src/builtins.h:483: static void Generate_InstantiateAsmJs(MacroAssembler* masm);
nit: Can we move this declaration up one line? Then it is not hiding in the
middle of the compilation stub generators for the other three tiers

https://codereview.chromium.org/2057403003/diff/180001/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/2057403003/diff/180001/src/compiler.cc#newcod...
src/compiler.cc:735: if (shared->HasAsmWasmData()) {
Can you elaborate on this guard. Is this needed for correctness? AFAICT the
runtime profiler should never be able to record ticks in the module code. So
this guard is confusing me. Is it possible to drop this guard?

https://codereview.chromium.org/2057403003/diff/180001/src/runtime/runtime-co...
File src/runtime/runtime-compiler.cc (right):

https://codereview.chromium.org/2057403003/diff/180001/src/runtime/runtime-co...
src/runtime/runtime-compiler.cc:93: RUNTIME_FUNCTION(Runtime_InstantiateAsmJs) {
nit: Lets move this up one function so that it is not hiding in the middle of
the deoptimizer support functions.

https://codereview.chromium.org/2057403003/diff/180001/src/runtime/runtime.h
File src/runtime/runtime.h (right):

https://codereview.chromium.org/2057403003/diff/180001/src/runtime/runtime.h#...
src/runtime/runtime.h:131: F(InstantiateAsmJs, -1, 1)
nit: s/-1/4/ because this runtime function always takes 4 arguments and does not
have var-args support.

https://codereview.chromium.org/2057403003/diff/180001/src/x64/builtins-x64.cc
File src/x64/builtins-x64.cc (right):

https://codereview.chromium.org/2057403003/diff/180001/src/x64/builtins-x64.c...
src/x64/builtins-x64.cc:112: __ Push(Operand(rsp, 10 * kPointerSize));
nit: Magic constant '10' is being magic. Can we express this with constants from
the CommonFrameConstants and friends? Or alternatively leave some asserts
against those constants in place to make sure the constant stays up to date.

[bradn, 2016-06-30 07:59:36]

PTAL

https://codereview.chromium.org/2057403003/diff/180001/src/builtins.cc
File src/builtins.cc (right):

https://codereview.chromium.org/2057403003/diff/180001/src/builtins.cc#newcode30
src/builtins.cc:30: #include "src/wasm/wasm-js.h"
On 2016/06/29 08:29:23, Michael Starzinger wrote:
> nit: Include shouldn't be needed anymore. Let's drop it.

Done.

https://codereview.chromium.org/2057403003/diff/180001/src/builtins.h
File src/builtins.h (right):

https://codereview.chromium.org/2057403003/diff/180001/src/builtins.h#newcode181
src/builtins.h:181: #if defined(V8_TARGET_ARCH_X64)
On 2016/06/29 08:29:23, Michael Starzinger wrote:
> We usually implement new features on all architectures in one go. Lets drop
> these #ifdef guards.

Done.

https://codereview.chromium.org/2057403003/diff/180001/src/builtins.h#newcode481
src/builtins.h:481: #if defined(V8_TARGET_ARCH_X64)
On 2016/06/29 08:29:23, Michael Starzinger wrote:
> Likewise.

Done.

https://codereview.chromium.org/2057403003/diff/180001/src/builtins.h#newcode483
src/builtins.h:483: static void Generate_InstantiateAsmJs(MacroAssembler* masm);
On 2016/06/29 08:29:23, Michael Starzinger wrote:
> nit: Can we move this declaration up one line? Then it is not hiding in the
> middle of the compilation stub generators for the other three tiers

Done.

https://codereview.chromium.org/2057403003/diff/180001/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/2057403003/diff/180001/src/compiler.cc#newcod...
src/compiler.cc:735: if (shared->HasAsmWasmData()) {
On 2016/06/29 08:29:23, Michael Starzinger wrote:
> Can you elaborate on this guard. Is this needed for correctness? AFAICT the
> runtime profiler should never be able to record ticks in the module code. So
> this guard is confusing me. Is it possible to drop this guard?

Not needed. Leftover from prior ignorance on my part.
Dropped.

https://codereview.chromium.org/2057403003/diff/180001/src/runtime/runtime-co...
File src/runtime/runtime-compiler.cc (right):

https://codereview.chromium.org/2057403003/diff/180001/src/runtime/runtime-co...
src/runtime/runtime-compiler.cc:93: RUNTIME_FUNCTION(Runtime_InstantiateAsmJs) {
On 2016/06/29 08:29:23, Michael Starzinger wrote:
> nit: Lets move this up one function so that it is not hiding in the middle of
> the deoptimizer support functions.

Done.

https://codereview.chromium.org/2057403003/diff/180001/src/runtime/runtime.h
File src/runtime/runtime.h (right):

https://codereview.chromium.org/2057403003/diff/180001/src/runtime/runtime.h#...
src/runtime/runtime.h:131: F(InstantiateAsmJs, -1, 1)
On 2016/06/29 08:29:23, Michael Starzinger wrote:
> nit: s/-1/4/ because this runtime function always takes 4 arguments and does
not
> have var-args support.

Done.

https://codereview.chromium.org/2057403003/diff/180001/src/x64/builtins-x64.cc
File src/x64/builtins-x64.cc (right):

https://codereview.chromium.org/2057403003/diff/180001/src/x64/builtins-x64.c...
src/x64/builtins-x64.cc:112: __ Push(Operand(rsp, 10 * kPointerSize));
On 2016/06/29 08:29:23, Michael Starzinger wrote:
> nit: Magic constant '10' is being magic. Can we express this with constants
from
> the CommonFrameConstants and friends? Or alternatively leave some asserts
> against those constants in place to make sure the constant stays up to date.

Done.

[bradn, 2016-06-30 08:26:47]

Description was changed from

==========
Hooking up asm-wasm conversion.

Directs 'use asm' traffic through asm-wasm conversion when --validate-asm is
passed.

Adds a builtin that handles the fallback to JS only for x64 for now.

BUG= https://bugs.chromium.org/p/v8/issues/detail?id=4203
TEST=asm-wasm
R=mstarzinger@chromium.org,titzer@chromium.org
LOG=N
==========

to

==========
Hooking up asm-wasm conversion.

Directs 'use asm' traffic through asm-wasm conversion when --validate-asm is
passed.

Adds a builtin that handles the fallback to JS.

BUG= https://bugs.chromium.org/p/v8/issues/detail?id=4203
TEST=asm-wasm
R=mstarzinger@chromium.org,titzer@chromium.org
LOG=N
==========

[Michael Starzinger, 2016-06-30 09:39:23]

LGTM on the plumbing outside of WASM. One last comment to address. Note that I
didn't review the changes inside of the "wasm" directory all too well. I'll
leave that part to Ben.

https://codereview.chromium.org/2057403003/diff/240001/src/arm/builtins-arm.cc
File src/arm/builtins-arm.cc (right):

https://codereview.chromium.org/2057403003/diff/240001/src/arm/builtins-arm.c...
src/arm/builtins-arm.cc:1461: GenerateTailCallToReturnedCode(masm,
Runtime::kCompileBaseline);
nit: s/CompileBaseline/CompileLazy here. Applies to all architectures. Sorry,
didn't spot this in my last round of comments. The following is the nomenclature
for compilation tiers:

CompileLazy      -> Tier 1 (either FCG or Ignition depending on config)
CompileBaseline  -> Tier 2 (always FCG, but only present in three-tier pipeline)
CompileOptimized -> Tier 3 (either CS or TF depending on config)

[titzer, 2016-06-30 09:56:03]

I'm OK with this CL generally, but wonder if we should just start an asmjs/
directory and dump typing-asm.*, asm-types.*, asm-wasm-builder.cc, and some of
the new logic in wasm-js.cc there?

https://codereview.chromium.org/2057403003/diff/240001/src/wasm/wasm-js.cc
File src/wasm/wasm-js.cc (right):

https://codereview.chromium.org/2057403003/diff/240001/src/wasm/wasm-js.cc#ne...
src/wasm/wasm-js.cc:487: bool WasmJs::ConvertAsmToWasm(ParseInfo* info, bool
fixed_signature,
The logic is basically OK here, but I think we should split out the
asm.js-related stuff into another file. The original intention of the wasm-js.cc
file was that it would be the place where the JS API for WASM lived. The fact
that we pulled out InstantiateModuleCommon here shows that some common code
maybe needs to move into wasm-module.cc and be shared by this wasm-js.cc and
asm-js.cc.

[Mircea Trofin, 2016-06-30 23:23:28]

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

[Mircea Trofin, 2016-06-30 23:23:30]

https://codereview.chromium.org/2057403003/diff/240001/src/typing-asm.cc
File src/typing-asm.cc (right):

https://codereview.chromium.org/2057403003/diff/240001/src/typing-asm.cc#newc...
src/typing-asm.cc:92: FAIL(fun, "only 3 parameter modules supported");
what's a 3-parameter module?

https://codereview.chromium.org/2057403003/diff/240001/src/wasm/wasm-js.cc
File src/wasm/wasm-js.cc (right):

https://codereview.chromium.org/2057403003/diff/240001/src/wasm/wasm-js.cc#ne...
src/wasm/wasm-js.cc:69: thrower->Error("Source must be an ArrayBuffer or
Uint8Array");
Super-nit: would the caller (js programmer, I assume) know what "source" is?
Should we call it source data or something like that?

https://codereview.chromium.org/2057403003/diff/240001/src/wasm/wasm-js.cc#ne...
src/wasm/wasm-js.cc:513: (*wasm_data)->set(1, *foreign_globals);
you could return MaybeHandle<FixedArray>, and then avoid having a out parameter
for wasm_data.

https://codereview.chromium.org/2057403003/diff/240001/src/wasm/wasm-js.cc#ne...
src/wasm/wasm-js.cc:549: factory->NewStringFromStaticChars("__foreign_init__");
Should you use one of the Internalize APIs, e.g. InternalizeOneByteString? I
think NewStringFromStaticChars will re-instantiate each time we hit here, so we
add GC pressure for no reason.

https://codereview.chromium.org/2057403003/diff/240001/src/wasm/wasm-js.cc#ne...
src/wasm/wasm-js.cc:560: for (int j = 0; j < foreign_globals->length(); j++) {
This is the asm.js initialization protocol? we could refactor (maybe in a
different CL) things a bit, I think, into decoding, instantiation, and
initialization.

https://codereview.chromium.org/2057403003/diff/240001/src/wasm/wasm-js.cc#ne...
src/wasm/wasm-js.cc:584: *result = maybe_module_object.ToHandleChecked();
same comment as above, maybe return MaybeHandle<Object>? It'd also be, I think,
more consistent, there are other APIs with this pattern. (if there are some bool
ones, perhaps we can agree on one way or the other, then progressively change? I
propose returning MaybeHandles wherever we produce JS heap objects in Wasm)

[bradn, 2016-07-01 01:06:09]

PTAL

https://codereview.chromium.org/2057403003/diff/240001/src/arm/builtins-arm.cc
File src/arm/builtins-arm.cc (right):

https://codereview.chromium.org/2057403003/diff/240001/src/arm/builtins-arm.c...
src/arm/builtins-arm.cc:1461: GenerateTailCallToReturnedCode(masm,
Runtime::kCompileBaseline);
On 2016/06/30 09:39:23, Michael Starzinger wrote:
> nit: s/CompileBaseline/CompileLazy here. Applies to all architectures. Sorry,
> didn't spot this in my last round of comments. The following is the
nomenclature
> for compilation tiers:
> 
> CompileLazy      -> Tier 1 (either FCG or Ignition depending on config)
> CompileBaseline  -> Tier 2 (always FCG, but only present in three-tier
pipeline)
> CompileOptimized -> Tier 3 (either CS or TF depending on config)

Ah, that's helpful.
Thanks.
Done

https://codereview.chromium.org/2057403003/diff/240001/src/typing-asm.cc
File src/typing-asm.cc (right):

https://codereview.chromium.org/2057403003/diff/240001/src/typing-asm.cc#newc...
src/typing-asm.cc:92: FAIL(fun, "only 3 parameter modules supported");
On 2016/06/30 23:23:29, Mircea Trofin wrote:
> what's a 3-parameter module?

Clarified.

https://codereview.chromium.org/2057403003/diff/240001/src/wasm/wasm-js.cc
File src/wasm/wasm-js.cc (right):

https://codereview.chromium.org/2057403003/diff/240001/src/wasm/wasm-js.cc#ne...
src/wasm/wasm-js.cc:69: thrower->Error("Source must be an ArrayBuffer or
Uint8Array");
On 2016/06/30 23:23:29, Mircea Trofin wrote:
> Super-nit: would the caller (js programmer, I assume) know what "source" is?
> Should we call it source data or something like that?

Reworded.

https://codereview.chromium.org/2057403003/diff/240001/src/wasm/wasm-js.cc#ne...
src/wasm/wasm-js.cc:487: bool WasmJs::ConvertAsmToWasm(ParseInfo* info, bool
fixed_signature,
On 2016/06/30 09:56:03, titzer wrote:
> The logic is basically OK here, but I think we should split out the
> asm.js-related stuff into another file. The original intention of the
wasm-js.cc
> file was that it would be the place where the JS API for WASM lived. The fact
> that we pulled out InstantiateModuleCommon here shows that some common code
> maybe needs to move into wasm-module.cc and be shared by this wasm-js.cc and
> asm-js.cc.

Done.

https://codereview.chromium.org/2057403003/diff/240001/src/wasm/wasm-js.cc#ne...
src/wasm/wasm-js.cc:513: (*wasm_data)->set(1, *foreign_globals);
On 2016/06/30 23:23:29, Mircea Trofin wrote:
> you could return MaybeHandle<FixedArray>, and then avoid having a out
parameter
> for wasm_data.

Done.

https://codereview.chromium.org/2057403003/diff/240001/src/wasm/wasm-js.cc#ne...
src/wasm/wasm-js.cc:549: factory->NewStringFromStaticChars("__foreign_init__");
On 2016/06/30 23:23:30, Mircea Trofin wrote:
> Should you use one of the Internalize APIs, e.g. InternalizeOneByteString? I
> think NewStringFromStaticChars will re-instantiate each time we hit here, so
we
> add GC pressure for no reason.

Done.

https://codereview.chromium.org/2057403003/diff/240001/src/wasm/wasm-js.cc#ne...
src/wasm/wasm-js.cc:560: for (int j = 0; j < foreign_globals->length(); j++) {
On 2016/06/30 23:23:29, Mircea Trofin wrote:
> This is the asm.js initialization protocol? we could refactor (maybe in a
> different CL) things a bit, I think, into decoding, instantiation, and
> initialization. 

Yes needs refactor.

https://codereview.chromium.org/2057403003/diff/240001/src/wasm/wasm-js.cc#ne...
src/wasm/wasm-js.cc:584: *result = maybe_module_object.ToHandleChecked();
On 2016/06/30 23:23:30, Mircea Trofin wrote:
> same comment as above, maybe return MaybeHandle<Object>? It'd also be, I
think,
> more consistent, there are other APIs with this pattern. (if there are some
bool
> ones, perhaps we can agree on one way or the other, then progressively change?
I
> propose returning MaybeHandles wherever we produce JS heap objects in Wasm)

Done.

[Mircea Trofin, 2016-07-01 05:17:02]

On 2016/07/01 01:06:09, bradn wrote:
> PTAL
> 
> https://codereview.chromium.org/2057403003/diff/240001/src/arm/builtins-arm.cc
> File src/arm/builtins-arm.cc (right):
> 
>
https://codereview.chromium.org/2057403003/diff/240001/src/arm/builtins-arm.c...
> src/arm/builtins-arm.cc:1461: GenerateTailCallToReturnedCode(masm,
> Runtime::kCompileBaseline);
> On 2016/06/30 09:39:23, Michael Starzinger wrote:
> > nit: s/CompileBaseline/CompileLazy here. Applies to all architectures.
Sorry,
> > didn't spot this in my last round of comments. The following is the
> nomenclature
> > for compilation tiers:
> > 
> > CompileLazy      -> Tier 1 (either FCG or Ignition depending on config)
> > CompileBaseline  -> Tier 2 (always FCG, but only present in three-tier
> pipeline)
> > CompileOptimized -> Tier 3 (either CS or TF depending on config)
> 
> Ah, that's helpful.
> Thanks.
> Done
> 
> https://codereview.chromium.org/2057403003/diff/240001/src/typing-asm.cc
> File src/typing-asm.cc (right):
> 
>
https://codereview.chromium.org/2057403003/diff/240001/src/typing-asm.cc#newc...
> src/typing-asm.cc:92: FAIL(fun, "only 3 parameter modules supported");
> On 2016/06/30 23:23:29, Mircea Trofin wrote:
> > what's a 3-parameter module?
> 
> Clarified.
> 
> https://codereview.chromium.org/2057403003/diff/240001/src/wasm/wasm-js.cc
> File src/wasm/wasm-js.cc (right):
> 
>
https://codereview.chromium.org/2057403003/diff/240001/src/wasm/wasm-js.cc#ne...
> src/wasm/wasm-js.cc:69: thrower->Error("Source must be an ArrayBuffer or
> Uint8Array");
> On 2016/06/30 23:23:29, Mircea Trofin wrote:
> > Super-nit: would the caller (js programmer, I assume) know what "source" is?
> > Should we call it source data or something like that?
> 
> Reworded.
> 
>
https://codereview.chromium.org/2057403003/diff/240001/src/wasm/wasm-js.cc#ne...
> src/wasm/wasm-js.cc:487: bool WasmJs::ConvertAsmToWasm(ParseInfo* info, bool
> fixed_signature,
> On 2016/06/30 09:56:03, titzer wrote:
> > The logic is basically OK here, but I think we should split out the
> > asm.js-related stuff into another file. The original intention of the
> wasm-js.cc
> > file was that it would be the place where the JS API for WASM lived. The
fact
> > that we pulled out InstantiateModuleCommon here shows that some common code
> > maybe needs to move into wasm-module.cc and be shared by this wasm-js.cc and
> > asm-js.cc.
> 
> Done.
> 
>
https://codereview.chromium.org/2057403003/diff/240001/src/wasm/wasm-js.cc#ne...
> src/wasm/wasm-js.cc:513: (*wasm_data)->set(1, *foreign_globals);
> On 2016/06/30 23:23:29, Mircea Trofin wrote:
> > you could return MaybeHandle<FixedArray>, and then avoid having a out
> parameter
> > for wasm_data.
> 
> Done.
> 
>
https://codereview.chromium.org/2057403003/diff/240001/src/wasm/wasm-js.cc#ne...
> src/wasm/wasm-js.cc:549:
factory->NewStringFromStaticChars("__foreign_init__");
> On 2016/06/30 23:23:30, Mircea Trofin wrote:
> > Should you use one of the Internalize APIs, e.g. InternalizeOneByteString? I
> > think NewStringFromStaticChars will re-instantiate each time we hit here, so
> we
> > add GC pressure for no reason.
> 
> Done.
> 
>
https://codereview.chromium.org/2057403003/diff/240001/src/wasm/wasm-js.cc#ne...
> src/wasm/wasm-js.cc:560: for (int j = 0; j < foreign_globals->length(); j++) {
> On 2016/06/30 23:23:29, Mircea Trofin wrote:
> > This is the asm.js initialization protocol? we could refactor (maybe in a
> > different CL) things a bit, I think, into decoding, instantiation, and
> > initialization. 
> 
> Yes needs refactor.
> 
>
https://codereview.chromium.org/2057403003/diff/240001/src/wasm/wasm-js.cc#ne...
> src/wasm/wasm-js.cc:584: *result = maybe_module_object.ToHandleChecked();
> On 2016/06/30 23:23:30, Mircea Trofin wrote:
> > same comment as above, maybe return MaybeHandle<Object>? It'd also be, I
> think,
> > more consistent, there are other APIs with this pattern. (if there are some
> bool
> > ones, perhaps we can agree on one way or the other, then progressively
change?
> I
> > propose returning MaybeHandles wherever we produce JS heap objects in Wasm)
> 
> Done.

lgtm

[bradn, 2016-07-01 05:24:55]

The CQ bit was checked by bradnelson@google.com

[bradn, 2016-07-01 05:24:55]

The patchset sent to the CQ was uploaded after l-g-t-m from
mstarzinger@chromium.org
Link to the patchset: https://codereview.chromium.org/2057403003/#ps280001
(title: "fix")

[commit-bot: I haz the power, 2016-07-01 05:25:00]

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-01 05:27:11]

Description was changed from

==========
Hooking up asm-wasm conversion.

Directs 'use asm' traffic through asm-wasm conversion when --validate-asm is
passed.

Adds a builtin that handles the fallback to JS.

BUG= https://bugs.chromium.org/p/v8/issues/detail?id=4203
TEST=asm-wasm
R=mstarzinger@chromium.org,titzer@chromium.org
LOG=N
==========

to

==========
Hooking up asm-wasm conversion.

Directs 'use asm' traffic through asm-wasm conversion when --validate-asm is
passed.

Adds a builtin that handles the fallback to JS.

BUG= https://bugs.chromium.org/p/v8/issues/detail?id=4203
TEST=asm-wasm
R=mstarzinger@chromium.org,titzer@chromium.org
LOG=N
==========

[commit-bot: I haz the power, 2016-07-01 05:27:12]

Committed patchset #15 (id:280001)

[commit-bot: I haz the power, 2016-07-01 05:28:53]

Description was changed from

==========
Hooking up asm-wasm conversion.

Directs 'use asm' traffic through asm-wasm conversion when --validate-asm is
passed.

Adds a builtin that handles the fallback to JS.

BUG= https://bugs.chromium.org/p/v8/issues/detail?id=4203
TEST=asm-wasm
R=mstarzinger@chromium.org,titzer@chromium.org
LOG=N
==========

to

==========
Hooking up asm-wasm conversion.

Directs 'use asm' traffic through asm-wasm conversion when --validate-asm is
passed.

Adds a builtin that handles the fallback to JS.

BUG= https://bugs.chromium.org/p/v8/issues/detail?id=4203
TEST=asm-wasm
R=mstarzinger@chromium.org,titzer@chromium.org
LOG=N

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

[commit-bot: I haz the power, 2016-07-01 05:28:55]

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

[titzer, 2016-07-01 06:51:00]

On 2016/07/01 05:28:55, commit-bot: I haz the power wrote:
> Patchset 15 (id:??) landed as
> https://crrev.com/f20323dce2a796f25eddec83c34b97127d046745
> Cr-Commit-Position: refs/heads/master@{#37470}

lgtm

[titzer, 2016-07-01 11:12:15]

https://codereview.chromium.org/2118733003

https://codereview.chromium.org/2057403003/diff/240001/src/wasm/wasm-js.cc
File src/wasm/wasm-js.cc (right):

https://codereview.chromium.org/2057403003/diff/240001/src/wasm/wasm-js.cc#ne...
src/wasm/wasm-js.cc:395: }
Why'd these flags go away? That accidentally shipped WASM by default. Fixing.

[Michael Starzinger, 2016-07-01 11:41:27]

https://codereview.chromium.org/2057403003/diff/240001/src/wasm/wasm-js.cc
File src/wasm/wasm-js.cc (right):

https://codereview.chromium.org/2057403003/diff/240001/src/wasm/wasm-js.cc#ne...
src/wasm/wasm-js.cc:395: }
On 2016/07/01 11:12:15, titzer wrote:
> Why'd these flags go away? That accidentally shipped WASM by default. Fixing.

+1. I also explicitly checked for these flags in my review and LGTMed the
version _with_ the flags in place.

[bradn, 2016-07-01 12:39:53]

https://codereview.chromium.org/2057403003/diff/240001/src/wasm/wasm-js.cc
File src/wasm/wasm-js.cc (right):

https://codereview.chromium.org/2057403003/diff/240001/src/wasm/wasm-js.cc#ne...
src/wasm/wasm-js.cc:395: }
On 2016/07/01 11:41:27, Michael Starzinger wrote:
> On 2016/07/01 11:12:15, titzer wrote:
> > Why'd these flags go away? That accidentally shipped WASM by default.
Fixing.
> 
> +1. I also explicitly checked for these flags in my review and LGTMed the
> version _with_ the flags in place.

AAAH. Drat.
Sorry about that.
When I moved stuff over to the asmjs dir, I forgot I'd changed how this gets
called, and reverted the whole file.

Thanks for fixing!