[wasm] Separate compilation from instantiation

[wasm] Separate compilation from instantiation

Compilation of wasm functions happens before instantiation. Imports are linked afterwards, at instantiation time. Globals and memory are also
allocated and then tied in via relocation at instantiation time.

This paves the way for implementing Wasm.compile, a prerequisite to
offering the compiled code serialization feature.

Currently, the WasmModule::Compile method just returns a fixed array
containing the code objects. More appropriate modeling of the compiled module to come.

Opportunistically centralized the logic on how to update memory
references, size, and globals, since that logic is the exact same on each
architecture, except for the actual storing of values back in the
instruction stream.

BUG=v8:5072
Committed: https://crrev.com/c1d01aea117c62759cc2be1805493ed59218558c
Cr-Commit-Position: refs/heads/master@{#37086}

[Mircea Trofin, 2016-06-14 00:28:34]

Patchset #9 (id:160001) has been deleted

[Mircea Trofin, 2016-06-16 05:40:35]

Patchset #10 (id:200001) has been deleted

[Mircea Trofin, 2016-06-16 05:40:48]

Patchset #8 (id:140001) has been deleted

[Mircea Trofin, 2016-06-16 06:03:24]

Description was changed from

==========
sepcomp


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

to

==========
[wasm] Separate compilation from instantiation

Compilation of wasm functions happens before instantiation. Imports are linked
afterwards, at instantiation time. Globals and memory are also
allocated and then tied in via relocation at instantiation time.

This paves the way for implementing Wasm.compile, a prerequisite to
offering the compiled code serialization feature.

Currently, the WasmModule::Compile method just returns a fixed array
containing the code objects. More appropriate modeling of the compiled module to
come.

Opportunistically centralized the logic on how to update memory 
references, size, and globals, since that logic is the exact same on each
architecture, except for the actual storing of values back in the
instruction stream. 

BUG=v8:5072
==========

[Mircea Trofin, 2016-06-16 06:03:25]

mtrofin@chromium.org changed reviewers:
+ bradnelson@chromium.org, titzer@chromium.org

[Mircea Trofin, 2016-06-16 06:05:05]

https://codereview.chromium.org/2056633002/diff/240001/src/x64/assembler-x64.cc
File src/x64/assembler-x64.cc (right):

https://codereview.chromium.org/2056633002/diff/240001/src/x64/assembler-x64....
src/x64/assembler-x64.cc:590: if (is_int8(src.value_) &&
RelocInfo::IsNone(src.rmode_)) {
This I copied from Deepti's https://codereview.chromium.org/2051043002. It is
needed here, too.

[bradnelson, 2016-06-16 17:46:54]

lgtm, sans questions/suggestions and red trybots :-)

https://codereview.chromium.org/2056633002/diff/260001/src/arm/assembler-arm.cc
File src/arm/assembler-arm.cc (right):

https://codereview.chromium.org/2056633002/diff/260001/src/arm/assembler-arm....
src/arm/assembler-arm.cc:257: void
RelocInfo::UncheckedUpdateWasmMemoryReference(Address address,
I'm fine with this name, but the local naming convention seems to deviate from
the style guide (which calls for local consistency). Either or.

https://codereview.chromium.org/2056633002/diff/260001/src/wasm/wasm-module.cc
File src/wasm/wasm-module.cc (right):

https://codereview.chromium.org/2056633002/diff/260001/src/wasm/wasm-module.c...
src/wasm/wasm-module.cc:162: static Handle<Code> CreatePlaceholder(Factory*
factory, uint32_t index,
The longer term plan is to nix the need for these, correct?

https://codereview.chromium.org/2056633002/diff/260001/src/wasm/wasm-module.c...
src/wasm/wasm-module.cc:286: int mask = 1 << RelocInfo::WASM_MEMORY_REFERENCE |
You know your operator precedence well :-)
FWIW, I notice a mix of whether 1 << x are parethesized in the codebase, but
this is fine.

https://codereview.chromium.org/2056633002/diff/260001/src/wasm/wasm-module.c...
src/wasm/wasm-module.cc:720: Handle<FixedArray> WasmModule::Compile(Isolate*
isolate) const {
CompileFunctions ?

https://codereview.chromium.org/2056633002/diff/260001/src/wasm/wasm-module.c...
src/wasm/wasm-module.cc:725: WasmModuleInstance
temp_instance_for_compilation(this);
Yuck.
As we talked about, we should clean up the module instance struct.

[bradnelson, 2016-06-16 17:46:54]

lgtm, sans questions/suggestions and red trybots :-)

https://codereview.chromium.org/2056633002/diff/260001/src/arm/assembler-arm.cc
File src/arm/assembler-arm.cc (right):

https://codereview.chromium.org/2056633002/diff/260001/src/arm/assembler-arm....
src/arm/assembler-arm.cc:257: void
RelocInfo::UncheckedUpdateWasmMemoryReference(Address address,
I'm fine with this name, but the local naming convention seems to deviate from
the style guide (which calls for local consistency). Either or.

https://codereview.chromium.org/2056633002/diff/260001/src/wasm/wasm-module.cc
File src/wasm/wasm-module.cc (right):

https://codereview.chromium.org/2056633002/diff/260001/src/wasm/wasm-module.c...
src/wasm/wasm-module.cc:162: static Handle<Code> CreatePlaceholder(Factory*
factory, uint32_t index,
The longer term plan is to nix the need for these, correct?

https://codereview.chromium.org/2056633002/diff/260001/src/wasm/wasm-module.c...
src/wasm/wasm-module.cc:286: int mask = 1 << RelocInfo::WASM_MEMORY_REFERENCE |
You know your operator precedence well :-)
FWIW, I notice a mix of whether 1 << x are parethesized in the codebase, but
this is fine.

https://codereview.chromium.org/2056633002/diff/260001/src/wasm/wasm-module.c...
src/wasm/wasm-module.cc:720: Handle<FixedArray> WasmModule::Compile(Isolate*
isolate) const {
CompileFunctions ?

https://codereview.chromium.org/2056633002/diff/260001/src/wasm/wasm-module.c...
src/wasm/wasm-module.cc:725: WasmModuleInstance
temp_instance_for_compilation(this);
Yuck.
As we talked about, we should clean up the module instance struct.

[titzer, 2016-06-16 23:12:15]

https://codereview.chromium.org/2056633002/diff/300001/src/assembler.cc
File src/assembler.cc (right):

https://codereview.chromium.org/2056633002/diff/300001/src/assembler.cc#newco...
src/assembler.cc:389: if (!(new_size == 0 || (new_base <= updated_reference &&
You can use V8_FATAL for this.

https://codereview.chromium.org/2056633002/diff/300001/src/wasm/wasm-module.cc
File src/wasm/wasm-module.cc (right):

https://codereview.chromium.org/2056633002/diff/300001/src/wasm/wasm-module.c...
src/wasm/wasm-module.cc:181: const std::vector<Handle<Code>>& to_link,
What does to_link mean? Is it the finished code objects?

https://codereview.chromium.org/2056633002/diff/300001/src/wasm/wasm-module.c...
src/wasm/wasm-module.cc:763: WasmLinker::LinkModuleFunctions(isolate,
Why not make this an instance method, since there is an instance available here?

Also, why do we need to run the linker? Doesn't the compiled code already
contain direct calls to the placeholder code?

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

https://codereview.chromium.org/2056633002/diff/300001/src/wasm/wasm-module.h...
src/wasm/wasm-module.h:230: uint32_t mem_size;  // size of the linear memory.
I think this needs to be bigger, probably uint64_t, because 4GB won't fit here.

[Mircea Trofin, 2016-06-16 23:43:11]

https://codereview.chromium.org/2056633002/diff/300001/src/wasm/wasm-module.cc
File src/wasm/wasm-module.cc (right):

https://codereview.chromium.org/2056633002/diff/300001/src/wasm/wasm-module.c...
src/wasm/wasm-module.cc:181: const std::vector<Handle<Code>>& to_link,
On 2016/06/16 23:12:14, titzer wrote:
> What does to_link mean? Is it the finished code objects?

It's functions referenced from unlinked. When we link the wasm functions among
themselves, the unlinked 
and to_link sets are the same. When we link wasm to js functions, the to_link
set is the imports

https://codereview.chromium.org/2056633002/diff/300001/src/wasm/wasm-module.c...
src/wasm/wasm-module.cc:763: WasmLinker::LinkModuleFunctions(isolate,
On 2016/06/16 23:12:14, titzer wrote:
> Why not make this an instance method, since there is an instance available
here?
> 

There doesn't seem to be anything the wasm linker instance would bring in, in
fact,
that change I have that removes placeholders and uses indices completely gets
rid of
linker as an object.

> Also, why do we need to run the linker? Doesn't the compiled code already
> contain direct calls to the placeholder code?

To link the module functions within themselves.

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

https://codereview.chromium.org/2056633002/diff/300001/src/wasm/wasm-module.h...
src/wasm/wasm-module.h:230: uint32_t mem_size;  // size of the linear memory.
On 2016/06/16 23:12:14, titzer wrote:
> I think this needs to be bigger, probably uint64_t, because 4GB won't fit
here.

Oh, you mean the specific case when we allocate 4GB. Hmm... You are correct that
we need 32+1 bytes for that size. I'll change it to uint64 then.

I now understand why size_t before. I'd prefer uint64_t rather, and checking the
size,
to keep things consistent accross architectures.

[titzer, 2016-06-17 00:23:42]

https://codereview.chromium.org/2056633002/diff/300001/src/wasm/wasm-module.cc
File src/wasm/wasm-module.cc (right):

https://codereview.chromium.org/2056633002/diff/300001/src/wasm/wasm-module.c...
src/wasm/wasm-module.cc:181: const std::vector<Handle<Code>>& to_link,
On 2016/06/16 23:43:11, Mircea Trofin wrote:
> On 2016/06/16 23:12:14, titzer wrote:
> > What does to_link mean? Is it the finished code objects?
> 
> It's functions referenced from unlinked. When we link the wasm functions among
> themselves, the unlinked 
> and to_link sets are the same. When we link wasm to js functions, the to_link
> set is the imports

how about code_targets as a name?

https://codereview.chromium.org/2056633002/diff/300001/src/wasm/wasm-module.c...
src/wasm/wasm-module.cc:763: WasmLinker::LinkModuleFunctions(isolate,
On 2016/06/16 23:43:11, Mircea Trofin wrote:
> On 2016/06/16 23:12:14, titzer wrote:
> > Why not make this an instance method, since there is an instance available
> here?
> > 
> 
> There doesn't seem to be anything the wasm linker instance would bring in, in
> fact,
> that change I have that removes placeholders and uses indices completely gets
> rid of
> linker as an object.
> 

That sounds good, but until then can we simply move those static methods out?
Since we already have many top-level helpers in the anonymous namespace.

> > Also, why do we need to run the linker? Doesn't the compiled code already
> > contain direct calls to the placeholder code?
> 
> To link the module functions within themselves. 

Ah, right. Ok.

[Mircea Trofin, 2016-06-17 00:39:30]

https://codereview.chromium.org/2056633002/diff/300001/src/wasm/wasm-module.cc
File src/wasm/wasm-module.cc (right):

https://codereview.chromium.org/2056633002/diff/300001/src/wasm/wasm-module.c...
src/wasm/wasm-module.cc:181: const std::vector<Handle<Code>>& to_link,
On 2016/06/17 00:23:42, titzer wrote:
> On 2016/06/16 23:43:11, Mircea Trofin wrote:
> > On 2016/06/16 23:12:14, titzer wrote:
> > > What does to_link mean? Is it the finished code objects?
> > 
> > It's functions referenced from unlinked. When we link the wasm functions
among
> > themselves, the unlinked 
> > and to_link sets are the same. When we link wasm to js functions, the
to_link
> > set is the imports
> 
> how about code_targets as a name?

That's a good name, I'll rename. Thanks!

https://codereview.chromium.org/2056633002/diff/300001/src/wasm/wasm-module.c...
src/wasm/wasm-module.cc:763: WasmLinker::LinkModuleFunctions(isolate,
On 2016/06/17 00:23:42, titzer wrote:
> On 2016/06/16 23:43:11, Mircea Trofin wrote:
> > On 2016/06/16 23:12:14, titzer wrote:
> > > Why not make this an instance method, since there is an instance available
> > here?
> > > 
> > 
> > There doesn't seem to be anything the wasm linker instance would bring in,
in
> > fact,
> > that change I have that removes placeholders and uses indices completely
gets
> > rid of
> > linker as an object.
> > 
> 
> That sounds good, but until then can we simply move those static methods out?
> Since we already have many top-level helpers in the anonymous namespace.
You mean move the statics out as stand-alone functions? Happy to - that's
actually
in-line with what the aforementioned subsequent change would do, anyway.
> 
> > > Also, why do we need to run the linker? Doesn't the compiled code already
> > > contain direct calls to the placeholder code?
> > 
> > To link the module functions within themselves. 
> 
> Ah, right. Ok.

[Mircea Trofin, 2016-06-17 17:16:31]

The CQ bit was checked by mtrofin@chromium.org

[Mircea Trofin, 2016-06-17 17:16:31]

The patchset sent to the CQ was uploaded after l-g-t-m from
bradnelson@chromium.org
Link to the patchset: https://codereview.chromium.org/2056633002/#ps380001
(title: "feedback")

[commit-bot: I haz the power, 2016-06-17 17:16:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2056633002/380001

[Mircea Trofin, 2016-06-17 17:16:47]

https://codereview.chromium.org/2056633002/diff/260001/src/arm/assembler-arm.cc
File src/arm/assembler-arm.cc (right):

https://codereview.chromium.org/2056633002/diff/260001/src/arm/assembler-arm....
src/arm/assembler-arm.cc:257: void
RelocInfo::UncheckedUpdateWasmMemoryReference(Address address,
On 2016/06/16 17:46:54, bradnelson wrote:
> I'm fine with this name, but the local naming convention seems to deviate from
> the style guide (which calls for local consistency). Either or.

Done.

https://codereview.chromium.org/2056633002/diff/260001/src/wasm/wasm-module.cc
File src/wasm/wasm-module.cc (right):

https://codereview.chromium.org/2056633002/diff/260001/src/wasm/wasm-module.c...
src/wasm/wasm-module.cc:162: static Handle<Code> CreatePlaceholder(Factory*
factory, uint32_t index,
On 2016/06/16 17:46:54, bradnelson wrote:
> The longer term plan is to nix the need for these, correct?

Yes. I plan on doing that after I have a full, round trip serialization to
IndexedDB solution.

https://codereview.chromium.org/2056633002/diff/260001/src/wasm/wasm-module.c...
src/wasm/wasm-module.cc:286: int mask = 1 << RelocInfo::WASM_MEMORY_REFERENCE |
On 2016/06/16 17:46:54, bradnelson wrote:
> You know your operator precedence well :-)
> FWIW, I notice a mix of whether 1 << x are parethesized in the codebase, but
> this is fine.

Fixed, because I prefer the readability and clarity of brackets, too.

https://codereview.chromium.org/2056633002/diff/260001/src/wasm/wasm-module.c...
src/wasm/wasm-module.cc:720: Handle<FixedArray> WasmModule::Compile(Isolate*
isolate) const {
On 2016/06/16 17:46:54, bradnelson wrote:
> CompileFunctions ?

Done.

https://codereview.chromium.org/2056633002/diff/260001/src/wasm/wasm-module.c...
src/wasm/wasm-module.cc:725: WasmModuleInstance
temp_instance_for_compilation(this);
On 2016/06/16 17:46:54, bradnelson wrote:
> Yuck.
> As we talked about, we should clean up the module instance struct.

Yup! :)

[commit-bot: I haz the power, 2016-06-17 17:19:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-17 17:19:51]

Try jobs failed on following builders:
  v8_presubmit on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/17498)

[Mircea Trofin, 2016-06-17 18:32:46]

PTAL

Note: I'll separately do something about the mem size as
uint64_t for the 4GB case. I want to add the appropriate 
testing, too.

[titzer, 2016-06-20 04:51:54]

lgtm

[Mircea Trofin, 2016-06-20 04:52:18]

The CQ bit was checked by mtrofin@chromium.org

[commit-bot: I haz the power, 2016-06-20 04:52:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2056633002/380001

[commit-bot: I haz the power, 2016-06-20 05:22:11]

Description was changed from

==========
[wasm] Separate compilation from instantiation

Compilation of wasm functions happens before instantiation. Imports are linked
afterwards, at instantiation time. Globals and memory are also
allocated and then tied in via relocation at instantiation time.

This paves the way for implementing Wasm.compile, a prerequisite to
offering the compiled code serialization feature.

Currently, the WasmModule::Compile method just returns a fixed array
containing the code objects. More appropriate modeling of the compiled module to
come.

Opportunistically centralized the logic on how to update memory 
references, size, and globals, since that logic is the exact same on each
architecture, except for the actual storing of values back in the
instruction stream. 

BUG=v8:5072
==========

to

==========
[wasm] Separate compilation from instantiation

Compilation of wasm functions happens before instantiation. Imports are linked
afterwards, at instantiation time. Globals and memory are also
allocated and then tied in via relocation at instantiation time.

This paves the way for implementing Wasm.compile, a prerequisite to
offering the compiled code serialization feature.

Currently, the WasmModule::Compile method just returns a fixed array
containing the code objects. More appropriate modeling of the compiled module to
come.

Opportunistically centralized the logic on how to update memory 
references, size, and globals, since that logic is the exact same on each
architecture, except for the actual storing of values back in the
instruction stream. 

BUG=v8:5072
==========

[commit-bot: I haz the power, 2016-06-20 05:22:12]

Committed patchset #17 (id:380001)

[commit-bot: I haz the power, 2016-06-20 05:23:45]

Description was changed from

==========
[wasm] Separate compilation from instantiation

Compilation of wasm functions happens before instantiation. Imports are linked
afterwards, at instantiation time. Globals and memory are also
allocated and then tied in via relocation at instantiation time.

This paves the way for implementing Wasm.compile, a prerequisite to
offering the compiled code serialization feature.

Currently, the WasmModule::Compile method just returns a fixed array
containing the code objects. More appropriate modeling of the compiled module to
come.

Opportunistically centralized the logic on how to update memory 
references, size, and globals, since that logic is the exact same on each
architecture, except for the actual storing of values back in the
instruction stream. 

BUG=v8:5072
==========

to

==========
[wasm] Separate compilation from instantiation

Compilation of wasm functions happens before instantiation. Imports are linked
afterwards, at instantiation time. Globals and memory are also
allocated and then tied in via relocation at instantiation time.

This paves the way for implementing Wasm.compile, a prerequisite to
offering the compiled code serialization feature.

Currently, the WasmModule::Compile method just returns a fixed array
containing the code objects. More appropriate modeling of the compiled module to
come.

Opportunistically centralized the logic on how to update memory
references, size, and globals, since that logic is the exact same on each
architecture, except for the actual storing of values back in the
instruction stream.

BUG=v8:5072

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

[commit-bot: I haz the power, 2016-06-20 05:23:47]

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

[Clemens Hammacher, 2016-06-20 07:46:10]

This CL made the reported code size drop to nearly zero:
https://chromeperf.appspot.com/report?sid=8e65fdc032014d9aec8cf189855dd252e3d...

In case the link does not work: Check
1) internal.client.v8/Haswell_x64/v8 / Wasm / AngryBots / CodeSize
2) internal.client.v8/Haswell_x64/v8 / Wasm / BananaBread / CodeSize

[Mircea Trofin, 2016-06-22 21:38:47]

On 2016/06/20 07:46:10, Clemens Hammacher wrote:
> This CL made the reported code size drop to nearly zero:
>
https://chromeperf.appspot.com/report?sid=8e65fdc032014d9aec8cf189855dd252e3d...
> 
> In case the link does not work: Check
> 1) internal.client.v8/Haswell_x64/v8 / Wasm / AngryBots / CodeSize
> 2) internal.client.v8/Haswell_x64/v8 / Wasm / BananaBread / CodeSize

Fixed in https://codereview.chromium.org/2079353002/