[wasm] simplify dependencies between graph builder and function decoder

Steps towards removing the dependency on ModuleEnv from the compiler.

This CL simplifies the relation between the wasm graph builder, the 
wasm decoder, and the wasm module they work on.

BUG=

Review-Url: https://codereview.chromium.org/2612643002
Cr-Commit-Position: refs/heads/master@{#42056}
Committed: https://chromium.googlesource.com/v8/v8/+/da70d7aa9f5313885704e82e5da6da98f15df1d6

[Mircea Trofin, 2017-01-03 22:29:39]

The CQ bit was checked by mtrofin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-03 22:29:43]

Dry run: 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, 2017-01-03 22:55:41]

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

[commit-bot: I haz the power, 2017-01-03 22:55:42]

Dry run: This issue passed the CQ dry run.

[Mircea Trofin, 2017-01-03 23:12:32]

The CQ bit was checked by mtrofin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-03 23:12:38]

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

[Mircea Trofin, 2017-01-03 23:19:39]

Description was changed from

==========
[wasm] graph builder and decoder work on the same module

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

to

==========
Steps towards removing the dependency on ModuleEnv from the compiler.

This CL simplifies the relation between the wasm graph builder, the 
wasm decoder, and the wasm module they work on.

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

[Mircea Trofin, 2017-01-03 23:19:40]

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

[commit-bot: I haz the power, 2017-01-03 23:28:27]

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

[commit-bot: I haz the power, 2017-01-03 23:28:28]

Dry run: Try jobs failed on following builders:
  v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...)
  v8_linux64_asan_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng_trig...)

[Mircea Trofin, 2017-01-03 23:42:57]

The CQ bit was checked by mtrofin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-03 23:42:59]

Dry run: 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, 2017-01-03 23:55:49]

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

[commit-bot: I haz the power, 2017-01-03 23:55:50]

Dry run: Try jobs failed on following builders:
  v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...)
  v8_linux64_asan_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng_trig...)
  v8_win_nosnap_shared_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...)
  v8_win_nosnap_shared_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...)

[bradnelson, 2017-01-03 23:59:54]

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

[bradnelson, 2017-01-03 23:59:55]

lgtm

I think if we can move to a place where we don't have all these null checks all
over the place that would be much better. It makes the code very bi-modal and is
hard to reason about. I wonder if perhaps consistently requiring a module, but
providing a testing mock module would be a better pattern to avoid building a
real module for function level tests.

https://codereview.chromium.org/2612643002/diff/40001/src/wasm/function-body-...
File src/wasm/function-body-decoder.cc (right):

https://codereview.chromium.org/2612643002/diff/40001/src/wasm/function-body-...
src/wasm/function-body-decoder.cc:228: if (module_ == nullptr || table_index >=
module_->function_tables.size()) {
Is this check needed?

https://codereview.chromium.org/2612643002/diff/40001/src/wasm/function-body-...
src/wasm/function-body-decoder.cc:363: // The full WASM decoder for bytecode.
Both verifies bytecode and generates
Clarify this is 'or' not and (i.e. it can be used to verify without building a
graph).

[bradnelson, 2017-01-04 00:05:50]

By the way, overall I feel like this area of the code keeps getting nudged
around at random. It would be good if we could get some consensus around the
direction of what the end state should look like.

[Mircea Trofin, 2017-01-04 00:08:28]

The CQ bit was checked by mtrofin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-04 00:08:39]

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

[Mircea Trofin, 2017-01-04 00:20:20]

On 2017/01/04 00:05:50, bradnelson wrote:
> By the way, overall I feel like this area of the code keeps getting nudged
> around at random. It would be good if we could get some consensus around the
> direction of what the end state should look like.

I agree. I'll form a better opinion after I peel off the compiler from
ModuleEnv, 
which should be general goodness (lower concept count), then come up with a
proposal, 
including considerations for the streamed compilation scenario.

[commit-bot: I haz the power, 2017-01-04 00:22:48]

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

[commit-bot: I haz the power, 2017-01-04 00:22:49]

Dry run: Try jobs failed on following builders:
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
  v8_linux64_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...)

[Mircea Trofin, 2017-01-04 00:30:08]

The CQ bit was checked by mtrofin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-04 00:30:18]

Dry run: 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, 2017-01-04 00:35:26]

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

[commit-bot: I haz the power, 2017-01-04 00:35:27]

Dry run: Try jobs failed on following builders:
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)

[titzer, 2017-01-04 03:32:42]

lgtm with comments

https://codereview.chromium.org/2612643002/diff/70001/src/wasm/function-body-...
File src/wasm/function-body-decoder.cc (right):

https://codereview.chromium.org/2612643002/diff/70001/src/wasm/function-body-...
src/wasm/function-body-decoder.cc:363: // The full WASM decoder for bytecode.
Verifies bytecode or generates
The comment should reflect that in the later case, it also verifies the bytecode
as a side-effect.

https://codereview.chromium.org/2612643002/diff/70001/src/wasm/function-body-...
src/wasm/function-body-decoder.cc:367: WasmFullDecoder(Zone* zone, const
FunctionBody& body)
I think it's better to have only a single constructor instead of constructor
chaining.

[Mircea Trofin, 2017-01-04 04:34:45]

The CQ bit was checked by mtrofin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-04 04:34:49]

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

[Mircea Trofin, 2017-01-04 04:55:33]

https://codereview.chromium.org/2612643002/diff/40001/src/wasm/function-body-...
File src/wasm/function-body-decoder.cc (right):

https://codereview.chromium.org/2612643002/diff/40001/src/wasm/function-body-...
src/wasm/function-body-decoder.cc:228: if (module_ == nullptr || table_index >=
module_->function_tables.size()) {
On 2017/01/03 23:59:55, bradnelson wrote:
> Is this check needed?

Yes, but can be written to be more specific. Done.

https://codereview.chromium.org/2612643002/diff/40001/src/wasm/function-body-...
src/wasm/function-body-decoder.cc:363: // The full WASM decoder for bytecode.
Both verifies bytecode and generates
On 2017/01/03 23:59:55, bradnelson wrote:
> Clarify this is 'or' not and (i.e. it can be used to verify without building a
> graph).

Done.

https://codereview.chromium.org/2612643002/diff/70001/src/wasm/function-body-...
File src/wasm/function-body-decoder.cc (right):

https://codereview.chromium.org/2612643002/diff/70001/src/wasm/function-body-...
src/wasm/function-body-decoder.cc:363: // The full WASM decoder for bytecode.
Verifies bytecode or generates
On 2017/01/04 03:32:41, titzer wrote:
> The comment should reflect that in the later case, it also verifies the
bytecode
> as a side-effect.

Good point - done.

https://codereview.chromium.org/2612643002/diff/70001/src/wasm/function-body-...
src/wasm/function-body-decoder.cc:367: WasmFullDecoder(Zone* zone, const
FunctionBody& body)
On 2017/01/04 03:32:41, titzer wrote:
> I think it's better to have only a single constructor instead of constructor
> chaining.

I tend to agree. I will make more changes in the upcoming days, which
should make the issue moot. Right now, it helps me understand the 
usages a bit better.

[commit-bot: I haz the power, 2017-01-04 05:12:17]

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

[commit-bot: I haz the power, 2017-01-04 05:12:18]

Dry run: This issue passed the CQ dry run.

[Mircea Trofin, 2017-01-04 05:12:23]

The CQ bit was checked by mtrofin@chromium.org

[Mircea Trofin, 2017-01-04 05:12:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
bradnelson@chromium.org, titzer@chromium.org
Link to the patchset: https://codereview.chromium.org/2612643002/#ps110001
(title: "removed references to ModuleEnv from function builder")

[commit-bot: I haz the power, 2017-01-04 05:12:27]

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, 2017-01-04 05:14:05]

CQ is committing da patch.

Bot data: {"patchset_id": 110001, "attempt_start_ts": 1483506743263240,
"parent_rev": "9a4f899dd996188c263d8abdaa69409e9222d163", "commit_rev":
"da70d7aa9f5313885704e82e5da6da98f15df1d6"}

[commit-bot: I haz the power, 2017-01-04 05:14:09]

Description was changed from

==========
Steps towards removing the dependency on ModuleEnv from the compiler.

This CL simplifies the relation between the wasm graph builder, the 
wasm decoder, and the wasm module they work on.

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

to

==========
Steps towards removing the dependency on ModuleEnv from the compiler.

This CL simplifies the relation between the wasm graph builder, the 
wasm decoder, and the wasm module they work on.

BUG=

Review-Url: https://codereview.chromium.org/2612643002
Cr-Commit-Position: refs/heads/master@{#42056}
Committed:
https://chromium.googlesource.com/v8/v8/+/da70d7aa9f5313885704e82e5da6da98f15...
==========

[commit-bot: I haz the power, 2017-01-04 05:14:10]

Committed patchset #7 (id:110001) as
https://chromium.googlesource.com/v8/v8/+/da70d7aa9f5313885704e82e5da6da98f15...