[wasm] Add loop assignment analysis.

[wasm] Add loop assignment analysis.

This CL implements loop assignment analysis, a pass over a loop's body
to record local variables that are assigned. This pre-pass is similar
to that done on the JavaScript AST for the same reason: avoid introducing
too many phis at loop headers when building a graph.

R=bradnelson@chromium.org,ahaas@chromium.org
BUG=

Committed: https://crrev.com/0ab1e56203e54dad6df332beea29e5621c13d254
Cr-Commit-Position: refs/heads/master@{#33486}

[bradnelson, 2016-01-22 06:28:33]

lgtm with some questions

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

https://codereview.chromium.org/1617723003/diff/20001/src/wasm/ast-decoder.cc...
src/wasm/ast-decoder.cc:105: WasmDecoder() : Decoder(nullptr, nullptr),
function_env_(nullptr) {}
Are you planning to pull this out into it's own file once all the loose
functions with the TODO's are inside?

https://codereview.chromium.org/1617723003/diff/20001/src/wasm/ast-decoder.cc...
src/wasm/ast-decoder.cc:1675: arity = 1;
Why bake in the arity again?
Pull the else block out instead?

https://codereview.chromium.org/1617723003/diff/20001/src/wasm/ast-decoder.cc...
src/wasm/ast-decoder.cc:1689: return assigned;
In a well formed program, shouldn't we always hit the return above?
A DCHECK here perhaps?

[ahaas, 2016-01-22 06:53:26]

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

https://codereview.chromium.org/1617723003/diff/20001/src/wasm/ast-decoder.cc...
src/wasm/ast-decoder.cc:1674: if (index < assigned->length())
assigned->Add(index);
How could index >= assigned-length() even be possible? Wouldn't there be an
error in LocalOperand()?

[titzer, 2016-01-23 16:20:37]

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

https://codereview.chromium.org/1617723003/diff/20001/src/wasm/ast-decoder.cc...
src/wasm/ast-decoder.cc:1674: if (index < assigned->length())
assigned->Add(index);
On 2016/01/22 06:53:26, ahaas wrote:
> How could index >= assigned-length() even be possible? Wouldn't there be an
> error in LocalOperand()?

This is because I plan to use it while decoding, when the decoder first hits the
kExprLoop bytecode. Thus this will run over unverified bytecode, which might
have errors. Also, LocalOperand() returns 0 on error, and there actually could
legally be 0 locals (which reminds me I should probably check that in the
decoder itself!).

https://codereview.chromium.org/1617723003/diff/20001/src/wasm/ast-decoder.cc...
src/wasm/ast-decoder.cc:1675: arity = 1;
On 2016/01/22 06:28:33, bradnelson wrote:
> Why bake in the arity again?
> Pull the else block out instead?

Ah, just a micro-optimization.

https://codereview.chromium.org/1617723003/diff/20001/src/wasm/ast-decoder.cc...
src/wasm/ast-decoder.cc:1689: return assigned;
On 2016/01/22 06:28:33, bradnelson wrote:
> In a well formed program, shouldn't we always hit the return above?
> A DCHECK here perhaps?

As mentioned above, I'm assuming not assuming the loop body is well-formed.

[titzer, 2016-01-25 10:25:35]

The CQ bit was checked by titzer@chromium.org

[titzer, 2016-01-25 10:25:35]

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/1617723003/#ps80001
(title: " ")

[commit-bot: I haz the power, 2016-01-25 10:25:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1617723003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1617723003/80001

[commit-bot: I haz the power, 2016-01-25 10:27:32]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-01-25 10:28:14]

Description was changed from

==========
[wasm] Add loop assignment analysis.

This CL implements loop assignment analysis, a pass over a loop's body
to record local variables that are assigned. This pre-pass is similar
to that done on the JavaScript AST for the same reason: avoid introducing
too many phis at loop headers when building a graph.

R=bradnelson@chromium.org,ahaas@chromium.org
BUG=
==========

to

==========
[wasm] Add loop assignment analysis.

This CL implements loop assignment analysis, a pass over a loop's body
to record local variables that are assigned. This pre-pass is similar
to that done on the JavaScript AST for the same reason: avoid introducing
too many phis at loop headers when building a graph.

R=bradnelson@chromium.org,ahaas@chromium.org
BUG=

Committed: https://crrev.com/0ab1e56203e54dad6df332beea29e5621c13d254
Cr-Commit-Position: refs/heads/master@{#33486}
==========

[commit-bot: I haz the power, 2016-01-25 10:28:14]

Patchset 5 (id:??) landed as
https://crrev.com/0ab1e56203e54dad6df332beea29e5621c13d254
Cr-Commit-Position: refs/heads/master@{#33486}