[wasm] Introduce WasmStackGuard builtin

[wasm] Introduce WasmStackGuard builtin

Instead of placing a runtime call to StackGuard in the compiled wasm
code, we just call the builtin, which is cheaper. By passing Smi::kZero
as context, we save even more code space and avoid embedding the
context in the code.
The WasmStackGuard builtin then calls the new WasmStackGuard runtime
function, which gets the context from the instance attached to the
calling wasm code, and then does the usual StackGuard logic.

For the unity benchmark in asm-wasm mode, generated code size reduces
from 63.0 to 61.6 MB (-2.1%).

R=titzer@chromium.org, ahaas@chromium.org, mstarzinger@chromium.org

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

[Clemens Hammacher, 2017-02-14 14:07:59]

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

[commit-bot: I haz the power, 2017-02-14 14:08:04]

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

[Clemens Hammacher, 2017-02-14 14:08:35]

https://codereview.chromium.org/2691993004/diff/1/BUILD.gn
File BUILD.gn (right):

https://codereview.chromium.org/2691993004/diff/1/BUILD.gn#newcode2569
BUILD.gn:2569: ":d8(//build/toolchain/linux:clang_x86)",
FYI: This change is forced by clang-format.

[commit-bot: I haz the power, 2017-02-14 14:09:54]

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

[commit-bot: I haz the power, 2017-02-14 14:09:54]

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_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/20975)
  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...)

[Clemens Hammacher, 2017-02-14 14:19:50]

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

[commit-bot: I haz the power, 2017-02-14 14:19:54]

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

[Michael Starzinger, 2017-02-14 14:43:23]

LGTM.

https://codereview.chromium.org/2691993004/diff/1/BUILD.gn
File BUILD.gn (right):

https://codereview.chromium.org/2691993004/diff/1/BUILD.gn#newcode2569
BUILD.gn:2569: ":d8(//build/toolchain/linux:clang_x86)",
On 2017/02/14 14:08:35, Clemens Hammacher wrote:
> FYI: This change is forced by clang-format.

Acknowledged. Same thing happened to me for one of my changes.

https://codereview.chromium.org/2691993004/diff/20001/src/builtins/builtins-w...
File src/builtins/builtins-wasm.cc (right):

https://codereview.chromium.org/2691993004/diff/20001/src/builtins/builtins-w...
src/builtins/builtins-wasm.cc:15: typedef CodeStubAssembler::ParameterMode
ParameterMode;
nit: The "ParameterMode" typedef seems to be unused.

[ahaas, 2017-02-14 14:45:39]

lgtm with nits

https://codereview.chromium.org/2691993004/diff/1/src/runtime/runtime-wasm.cc
File src/runtime/runtime-wasm.cc (right):

https://codereview.chromium.org/2691993004/diff/1/src/runtime/runtime-wasm.cc...
src/runtime/runtime-wasm.cc:67: ->compiled_module()
Could you use GetWasmInstanceOnStackTop(isolate) here as well?

https://codereview.chromium.org/2691993004/diff/1/src/runtime/runtime-wasm.cc...
src/runtime/runtime-wasm.cc:184:
isolate->set_context(*instance->compiled_module()->native_context());
nit: Can you add a DCHECK_NULL(isolate->context()); here?

https://codereview.chromium.org/2691993004/diff/1/src/runtime/runtime-wasm.cc...
src/runtime/runtime-wasm.cc:196:
isolate->set_context(instance->compiled_module()->ptr_to_native_context());
nit: Can you add a DCHECK_NULL(isolate->context()); here?

https://codereview.chromium.org/2691993004/diff/20001/src/builtins/builtins-w...
File src/builtins/builtins-wasm.cc (right):

https://codereview.chromium.org/2691993004/diff/20001/src/builtins/builtins-w...
src/builtins/builtins-wasm.cc:8: #include "src/wasm/wasm-module.h"
Do we need all these includes here?

https://codereview.chromium.org/2691993004/diff/20001/src/builtins/builtins-w...
src/builtins/builtins-wasm.cc:19: Node* context = SmiConstant(0);
I wonder if SmiConstant(Smi::kZero) would be better here.

[commit-bot: I haz the power, 2017-02-14 15:07:42]

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

[commit-bot: I haz the power, 2017-02-14 15:07:42]

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

[Clemens Hammacher, 2017-02-14 17:20:30]

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

[commit-bot: I haz the power, 2017-02-14 17:20:33]

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

[Clemens Hammacher, 2017-02-14 17:23:15]

https://codereview.chromium.org/2691993004/diff/1/src/runtime/runtime-wasm.cc
File src/runtime/runtime-wasm.cc (right):

https://codereview.chromium.org/2691993004/diff/1/src/runtime/runtime-wasm.cc...
src/runtime/runtime-wasm.cc:184:
isolate->set_context(*instance->compiled_module()->native_context());
On 2017/02/14 at 14:45:38, ahaas wrote:
> nit: Can you add a DCHECK_NULL(isolate->context()); here?

Done.

https://codereview.chromium.org/2691993004/diff/1/src/runtime/runtime-wasm.cc...
src/runtime/runtime-wasm.cc:196:
isolate->set_context(instance->compiled_module()->ptr_to_native_context());
On 2017/02/14 at 14:45:38, ahaas wrote:
> nit: Can you add a DCHECK_NULL(isolate->context()); here?

Done.

https://codereview.chromium.org/2691993004/diff/20001/src/builtins/builtins-w...
File src/builtins/builtins-wasm.cc (right):

https://codereview.chromium.org/2691993004/diff/20001/src/builtins/builtins-w...
src/builtins/builtins-wasm.cc:8: #include "src/wasm/wasm-module.h"
On 2017/02/14 at 14:45:38, ahaas wrote:
> Do we need all these includes here?

Thanks for the hint. Removed three of them.

https://codereview.chromium.org/2691993004/diff/20001/src/builtins/builtins-w...
src/builtins/builtins-wasm.cc:15: typedef CodeStubAssembler::ParameterMode
ParameterMode;
On 2017/02/14 at 14:43:22, Michael Starzinger wrote:
> nit: The "ParameterMode" typedef seems to be unused.

Done, also CodeAssemblerState.

https://codereview.chromium.org/2691993004/diff/20001/src/builtins/builtins-w...
src/builtins/builtins-wasm.cc:19: Node* context = SmiConstant(0);
On 2017/02/14 at 14:45:39, ahaas wrote:
> I wonder if SmiConstant(Smi::kZero) would be better here.

Done.

[Clemens Hammacher, 2017-02-14 17:25:30]

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

[commit-bot: I haz the power, 2017-02-14 17:25:37]

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-02-14 18:03:24]

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

[commit-bot: I haz the power, 2017-02-14 18:03:25]

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

[Clemens Hammacher, 2017-02-14 18:14:13]

I opened a bug for the GCMole crash:
https://bugs.chromium.org/p/v8/issues/detail?id=5970

[Clemens Hammacher, 2017-02-16 11:41:51]

Rebase

[Clemens Hammacher, 2017-02-16 11:41:52]

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

[commit-bot: I haz the power, 2017-02-16 11:41:54]

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

[titzer, 2017-02-16 11:52:57]

On 2017/02/16 11:41:51, Clemens Hammacher wrote:
> Rebase

lgtm

[commit-bot: I haz the power, 2017-02-16 12:30:10]

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

[commit-bot: I haz the power, 2017-02-16 12:30:11]

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

[Clemens Hammacher, 2017-02-16 19:04:49]

Aha! gcmole is not crashing any more :)
Will land this tomorrow.

[Clemens Hammacher, 2017-02-16 20:54:57]

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

[commit-bot: I haz the power, 2017-02-16 20:55:05]

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-02-16 21:23:51]

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

[commit-bot: I haz the power, 2017-02-16 21:23:52]

Dry run: This issue passed the CQ dry run.

[Clemens Hammacher, 2017-02-17 11:36:04]

The CQ bit was checked by clemensh@chromium.org

[Clemens Hammacher, 2017-02-17 11:36:04]

The patchset sent to the CQ was uploaded after l-g-t-m from ahaas@chromium.org,
mstarzinger@chromium.org, titzer@chromium.org
Link to the patchset: https://codereview.chromium.org/2691993004/#ps100001
(title: "Rebase")

[commit-bot: I haz the power, 2017-02-17 11:36:13]

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-02-17 11:37:51]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1487331364130940,
"parent_rev": "a8e3925e7ab630cd9c336a737994e21343843303", "commit_rev":
"b6bfe7b91169fdb6da7b40b4d1361e9e0d5690bc"}

[commit-bot: I haz the power, 2017-02-17 11:37:56]

Description was changed from

==========
[wasm] Introduce WasmStackGuard builtin

Instead of placing a runtime call to StackGuard in the compiled wasm
code, we just call the builtin, which is cheaper. By passing Smi::kZero
as context, we save even more code space and avoid embedding the
context in the code.
The WasmStackGuard builtin then calls the new WasmStackGuard runtime
function, which gets the context from the instance attached to the
calling wasm code, and then does the usual StackGuard logic.

For the unity benchmark in asm-wasm mode, generated code size reduces
from 63.0 to 61.6 MB (-2.1%).

R=titzer@chromium.org, ahaas@chromium.org, mstarzinger@chromium.org
==========

to

==========
[wasm] Introduce WasmStackGuard builtin

Instead of placing a runtime call to StackGuard in the compiled wasm
code, we just call the builtin, which is cheaper. By passing Smi::kZero
as context, we save even more code space and avoid embedding the
context in the code.
The WasmStackGuard builtin then calls the new WasmStackGuard runtime
function, which gets the context from the instance attached to the
calling wasm code, and then does the usual StackGuard logic.

For the unity benchmark in asm-wasm mode, generated code size reduces
from 63.0 to 61.6 MB (-2.1%).

R=titzer@chromium.org, ahaas@chromium.org, mstarzinger@chromium.org

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

[commit-bot: I haz the power, 2017-02-17 11:37:57]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/v8/v8/+/b6bfe7b91169fdb6da7b40b4d1361e9e0d5...