[wasm] Attach loop assignment analysis behind a flag.

[wasm] Attach loop assignment analysis behind a flag.

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

Committed: https://crrev.com/421a67b0f4c465f56b4408a381c709c26edcd29f
Cr-Commit-Position: refs/heads/master@{#34744}

[Mircea Trofin, 2016-03-11 17:22:19]

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

[Mircea Trofin, 2016-03-11 17:22:19]

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

https://codereview.chromium.org/1789683002/diff/1/src/wasm/ast-decoder.cc#new...
src/wasm/ast-decoder.cc:1473: if (assigned) {
Can this ever be nullptr?

Also, style nit: if (assigned != nullptr)

https://codereview.chromium.org/1789683002/diff/1/src/wasm/ast-decoder.cc#new...
src/wasm/ast-decoder.cc:1474: // Only introduce phis for variables assigned in
this loop.
If we have unverified code and an out of bounds operand index, in
AnalyzeLoopAssignment, does that mean that can't be a phi operand?

[titzer, 2016-03-14 10:24:47]

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

https://codereview.chromium.org/1789683002/diff/1/src/wasm/ast-decoder.cc#new...
src/wasm/ast-decoder.cc:1473: if (assigned) {
On 2016/03/11 17:22:19, Mircea Trofin wrote:
> Can this ever be nullptr?
> 
> Also, style nit: if (assigned != nullptr) 

Yes, there is a check in AnalyzeLoopAssignment that pc points to kExprLoop, and
in that case it returns NULL. Defensive programming.

Done.

https://codereview.chromium.org/1789683002/diff/1/src/wasm/ast-decoder.cc#new...
src/wasm/ast-decoder.cc:1474: // Only introduce phis for variables assigned in
this loop.
On 2016/03/11 17:22:19, Mircea Trofin wrote:
> If we have unverified code and an out of bounds operand index, in
> AnalyzeLoopAssignment, does that mean that can't be a phi operand?

Out-of-range variable indexes will be ignored by AnalyzeLoopAssignment. Later
when the main decoding loop processes those variable references, they will fail
as normal.

[Mircea Trofin, 2016-03-14 13:28:30]

On 2016/03/14 10:24:47, titzer wrote:
> https://codereview.chromium.org/1789683002/diff/1/src/wasm/ast-decoder.cc
> File src/wasm/ast-decoder.cc (right):
> 
>
https://codereview.chromium.org/1789683002/diff/1/src/wasm/ast-decoder.cc#new...
> src/wasm/ast-decoder.cc:1473: if (assigned) {
> On 2016/03/11 17:22:19, Mircea Trofin wrote:
> > Can this ever be nullptr?
> > 
> > Also, style nit: if (assigned != nullptr) 
> 
> Yes, there is a check in AnalyzeLoopAssignment that pc points to kExprLoop,
and
> in that case it returns NULL. Defensive programming.
> 
> Done.
> 
>
https://codereview.chromium.org/1789683002/diff/1/src/wasm/ast-decoder.cc#new...
> src/wasm/ast-decoder.cc:1474: // Only introduce phis for variables assigned in
> this loop.
> On 2016/03/11 17:22:19, Mircea Trofin wrote:
> > If we have unverified code and an out of bounds operand index, in
> > AnalyzeLoopAssignment, does that mean that can't be a phi operand?
> 
> Out-of-range variable indexes will be ignored by AnalyzeLoopAssignment. Later
> when the main decoding loop processes those variable references, they will
fail
> as normal.

lgtm, with 1 comment

[Mircea Trofin, 2016-03-14 13:28:38]

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

https://codereview.chromium.org/1789683002/diff/1/src/wasm/ast-decoder.cc#new...
src/wasm/ast-decoder.cc:1474: // Only introduce phis for variables assigned in
this loop.
On 2016/03/14 10:24:47, titzer wrote:
> On 2016/03/11 17:22:19, Mircea Trofin wrote:
> > If we have unverified code and an out of bounds operand index, in
> > AnalyzeLoopAssignment, does that mean that can't be a phi operand?
> 
> Out-of-range variable indexes will be ignored by AnalyzeLoopAssignment. Later
> when the main decoding loop processes those variable references, they will
fail
> as normal.

Could you add a comment about that?

[titzer, 2016-03-14 14:11:45]

The CQ bit was checked by titzer@chromium.org

[titzer, 2016-03-14 14:11:45]

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/1789683002/#ps40001
(title: " ")

[commit-bot: I haz the power, 2016-03-14 14:11:51]

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

[commit-bot: I haz the power, 2016-03-14 14:13:27]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-03-14 14:14:25]

Description was changed from

==========
[wasm] Attach loop assignment analysis behind a flag.

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

to

==========
[wasm] Attach loop assignment analysis behind a flag.

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

Committed: https://crrev.com/421a67b0f4c465f56b4408a381c709c26edcd29f
Cr-Commit-Position: refs/heads/master@{#34744}
==========

[commit-bot: I haz the power, 2016-03-14 14:14:26]

Patchset 3 (id:??) landed as
https://crrev.com/421a67b0f4c465f56b4408a381c709c26edcd29f
Cr-Commit-Position: refs/heads/master@{#34744}