[wasm] Add stack checks to loops.

[wasm] Add stack checks to loops.

Stack checks in loops allows to interrupt loops.

BUG=cctest/test-run-wasm-module/TestInterruptLoop
R=titzer@chromium.org, bradnelson@chromium.org

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

[ahaas, 2016-10-11 18:12:45]

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

[commit-bot: I haz the power, 2016-10-11 18:12:56]

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, 2016-10-11 18:21:04]

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

[commit-bot: I haz the power, 2016-10-11 18:21:05]

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

[titzer, 2016-10-11 18:44:34]

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

https://codereview.chromium.org/2405293002/diff/1/src/wasm/ast-decoder.cc#new...
src/wasm/ast-decoder.cc:687: PrepareForLoop(pc_, finish_try_env);
Can we make PrepareForLoop() return a new environment, the one that should be
used for the loop body?

[titzer, 2016-10-11 18:48:54]

https://codereview.chromium.org/2405293002/diff/1/src/compiler/wasm-compiler.h
File src/compiler/wasm-compiler.h (right):

https://codereview.chromium.org/2405293002/diff/1/src/compiler/wasm-compiler....
src/compiler/wasm-compiler.h:145: void StackCheck(wasm::WasmCodePosition
position, Node** effect = nullptr,
If we take the other suggestion in ast-decoder.cc of making PrepareForLoop()
return a new environment, then I think we can keep this method as it was (i.e.
changing the current control and effect dependencies).

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

https://codereview.chromium.org/2405293002/diff/1/src/wasm/wasm-module.h#newc...
src/wasm/wasm-module.h:54: enum WasmInstanceObjectFields {
Not sure why these internal fields need to be exposed now.

[ahaas, 2016-10-12 10:56:47]

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

[commit-bot: I haz the power, 2016-10-12 10:56:54]

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, 2016-10-12 11:31:32]

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

[commit-bot: I haz the power, 2016-10-12 11:31:33]

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...)

[ahaas, 2016-10-12 18:01:06]

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

[commit-bot: I haz the power, 2016-10-12 18:01:12]

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

[ahaas, 2016-10-12 18:03:58]

PTAL. I set the memory in the test to volatile now so that the C++ compiler does
not optimize away the synchronization.

https://codereview.chromium.org/2405293002/diff/1/src/compiler/wasm-compiler.h
File src/compiler/wasm-compiler.h (right):

https://codereview.chromium.org/2405293002/diff/1/src/compiler/wasm-compiler....
src/compiler/wasm-compiler.h:145: void StackCheck(wasm::WasmCodePosition
position, Node** effect = nullptr,
On 2016/10/11 at 18:48:54, titzer wrote:
> If we take the other suggestion in ast-decoder.cc of making PrepareForLoop()
return a new environment, then I think we can keep this method as it was (i.e.
changing the current control and effect dependencies).

I think that's not possible. {effect_} and {control_} is not set before {SetEnv}
is called in the ast-decoder, which is after the call to {StackCheck}.

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

https://codereview.chromium.org/2405293002/diff/1/src/wasm/ast-decoder.cc#new...
src/wasm/ast-decoder.cc:687: PrepareForLoop(pc_, finish_try_env);
On 2016/10/11 at 18:44:33, titzer wrote:
> Can we make PrepareForLoop() return a new environment, the one that should be
used for the loop body?

Done.

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

https://codereview.chromium.org/2405293002/diff/1/src/wasm/wasm-module.h#newc...
src/wasm/wasm-module.h:54: enum WasmInstanceObjectFields {
On 2016/10/11 at 18:48:54, titzer wrote:
> Not sure why these internal fields need to be exposed now.

I do not expose them anymore. Instead I make GetInstanceMemory in wasm-module.h
accessible. =

[commit-bot: I haz the power, 2016-10-12 18:04:12]

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

[commit-bot: I haz the power, 2016-10-12 18:04:13]

Dry run: Try jobs failed on following builders:
  v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...)
  v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)
  v8_linux_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)

[ahaas, 2016-10-12 18:15:12]

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

[commit-bot: I haz the power, 2016-10-12 18:15:14]

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, 2016-10-12 18:39:59]

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

[commit-bot: I haz the power, 2016-10-12 18:40:00]

Dry run: This issue passed the CQ dry run.

[titzer, 2016-10-12 19:12:03]

On 2016/10/12 18:03:58, ahaas wrote:
> PTAL. I set the memory in the test to volatile now so that the C++ compiler
does
> not optimize away the synchronization.
> 
> https://codereview.chromium.org/2405293002/diff/1/src/compiler/wasm-compiler.h
> File src/compiler/wasm-compiler.h (right):
> 
>
https://codereview.chromium.org/2405293002/diff/1/src/compiler/wasm-compiler....
> src/compiler/wasm-compiler.h:145: void StackCheck(wasm::WasmCodePosition
> position, Node** effect = nullptr,
> On 2016/10/11 at 18:48:54, titzer wrote:
> > If we take the other suggestion in ast-decoder.cc of making PrepareForLoop()
> return a new environment, then I think we can keep this method as it was (i.e.
> changing the current control and effect dependencies).
> 
> I think that's not possible. {effect_} and {control_} is not set before
{SetEnv}
> is called in the ast-decoder, which is after the call to {StackCheck}.
> 
> https://codereview.chromium.org/2405293002/diff/1/src/wasm/ast-decoder.cc
> File src/wasm/ast-decoder.cc (right):
> 
>
https://codereview.chromium.org/2405293002/diff/1/src/wasm/ast-decoder.cc#new...
> src/wasm/ast-decoder.cc:687: PrepareForLoop(pc_, finish_try_env);
> On 2016/10/11 at 18:44:33, titzer wrote:
> > Can we make PrepareForLoop() return a new environment, the one that should
be
> used for the loop body?
> 
> Done.
> 
> https://codereview.chromium.org/2405293002/diff/1/src/wasm/wasm-module.h
> File src/wasm/wasm-module.h (right):
> 
>
https://codereview.chromium.org/2405293002/diff/1/src/wasm/wasm-module.h#newc...
> src/wasm/wasm-module.h:54: enum WasmInstanceObjectFields {
> On 2016/10/11 at 18:48:54, titzer wrote:
> > Not sure why these internal fields need to be exposed now.
> 
> I do not expose them anymore. Instead I make GetInstanceMemory in
wasm-module.h
> accessible. =

lgtm

Please keep an eye on the perf bots, as this will undoubtedly cause some perf
regression.

[titzer, 2016-10-12 19:12:03]

On 2016/10/12 18:03:58, ahaas wrote:
> PTAL. I set the memory in the test to volatile now so that the C++ compiler
does
> not optimize away the synchronization.
> 
> https://codereview.chromium.org/2405293002/diff/1/src/compiler/wasm-compiler.h
> File src/compiler/wasm-compiler.h (right):
> 
>
https://codereview.chromium.org/2405293002/diff/1/src/compiler/wasm-compiler....
> src/compiler/wasm-compiler.h:145: void StackCheck(wasm::WasmCodePosition
> position, Node** effect = nullptr,
> On 2016/10/11 at 18:48:54, titzer wrote:
> > If we take the other suggestion in ast-decoder.cc of making PrepareForLoop()
> return a new environment, then I think we can keep this method as it was (i.e.
> changing the current control and effect dependencies).
> 
> I think that's not possible. {effect_} and {control_} is not set before
{SetEnv}
> is called in the ast-decoder, which is after the call to {StackCheck}.
> 
> https://codereview.chromium.org/2405293002/diff/1/src/wasm/ast-decoder.cc
> File src/wasm/ast-decoder.cc (right):
> 
>
https://codereview.chromium.org/2405293002/diff/1/src/wasm/ast-decoder.cc#new...
> src/wasm/ast-decoder.cc:687: PrepareForLoop(pc_, finish_try_env);
> On 2016/10/11 at 18:44:33, titzer wrote:
> > Can we make PrepareForLoop() return a new environment, the one that should
be
> used for the loop body?
> 
> Done.
> 
> https://codereview.chromium.org/2405293002/diff/1/src/wasm/wasm-module.h
> File src/wasm/wasm-module.h (right):
> 
>
https://codereview.chromium.org/2405293002/diff/1/src/wasm/wasm-module.h#newc...
> src/wasm/wasm-module.h:54: enum WasmInstanceObjectFields {
> On 2016/10/11 at 18:48:54, titzer wrote:
> > Not sure why these internal fields need to be exposed now.
> 
> I do not expose them anymore. Instead I make GetInstanceMemory in
wasm-module.h
> accessible. =

lgtm

Please keep an eye on the perf bots, as this will undoubtedly cause some perf
regression.

[titzer, 2016-10-12 19:16:26]

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

https://codereview.chromium.org/2405293002/diff/60001/src/wasm/ast-decoder.cc...
src/wasm/ast-decoder.cc:687: SetEnv("loop:start", PrepareForLoop(pc_,
finish_try_env));
Please introduce a local variable here to make it clear that the environment
returned by PrepareForLoop is the body environment.

https://codereview.chromium.org/2405293002/diff/60001/src/wasm/ast-decoder.cc...
src/wasm/ast-decoder.cc:1613: SsaEnv* BuildLoopStackCheck(SsaEnv* env) {
You can inline this.

https://codereview.chromium.org/2405293002/diff/60001/src/wasm/ast-decoder.cc...
src/wasm/ast-decoder.cc:1620: if (!env->go()) return
BuildLoopStackCheck(Split(env));
You shouldn't build the stack check if the ssa environment isn't reachable.

[ahaas, 2016-10-13 09:19:51]

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

[commit-bot: I haz the power, 2016-10-13 09:20:02]

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, 2016-10-13 09:46:47]

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

[commit-bot: I haz the power, 2016-10-13 09:46:48]

Dry run: This issue passed the CQ dry run.

[ahaas, 2016-10-13 10:51:56]

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

https://codereview.chromium.org/2405293002/diff/60001/src/wasm/ast-decoder.cc...
src/wasm/ast-decoder.cc:687: SetEnv("loop:start", PrepareForLoop(pc_,
finish_try_env));
On 2016/10/12 at 19:16:25, titzer wrote:
> Please introduce a local variable here to make it clear that the environment
returned by PrepareForLoop is the body environment.

Done.

https://codereview.chromium.org/2405293002/diff/60001/src/wasm/ast-decoder.cc...
src/wasm/ast-decoder.cc:1613: SsaEnv* BuildLoopStackCheck(SsaEnv* env) {
On 2016/10/12 at 19:16:25, titzer wrote:
> You can inline this.

Done.

https://codereview.chromium.org/2405293002/diff/60001/src/wasm/ast-decoder.cc...
src/wasm/ast-decoder.cc:1620: if (!env->go()) return
BuildLoopStackCheck(Split(env));
On 2016/10/12 at 19:16:25, titzer wrote:
> You shouldn't build the stack check if the ssa environment isn't reachable.

Done.

[ahaas, 2016-10-13 10:52:27]

The CQ bit was checked by ahaas@chromium.org

[ahaas, 2016-10-13 10:52:27]

The patchset sent to the CQ was uploaded after l-g-t-m from titzer@chromium.org
Link to the patchset: https://codereview.chromium.org/2405293002/#ps80001
(title: "comments addressed")

[commit-bot: I haz the power, 2016-10-13 10:52:36]

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-10-13 10:56:51]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-10-13 10:57:06]

Description was changed from

==========
[wasm] Add stack checks to loops.

Stack checks in loops allows to interrupt loops.

BUG=cctest/test-run-wasm-module/TestInterruptLoop
R=titzer@chromium.org, bradnelson@chromium.org
==========

to

==========
[wasm] Add stack checks to loops.

Stack checks in loops allows to interrupt loops.

BUG=cctest/test-run-wasm-module/TestInterruptLoop
R=titzer@chromium.org, bradnelson@chromium.org

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

[commit-bot: I haz the power, 2016-10-13 10:57:07]

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