[wasm] Introduce the TrapIf and TrapUnless operators to generate trap code.

[wasm] Introduce the TrapIf and TrapUnless operators to generate trap code.

Some instructions in WebAssembly trap for some inputs, which means that the
execution is terminated and (at least at the moment) a JavaScript exception is
thrown. Examples for traps are out-of-bounds memory accesses, or integer
divisions by zero.

Without the TrapIf and TrapUnless operators trap check in WebAssembly introduces 5
TurboFan nodes (branch, if_true, if_false, trap-reason constant, trap-position
constant), in addition to the trap condition itself. Additionally, each
WebAssembly function has four TurboFan nodes (merge, effect_phi, 2 phis) whose
number of inputs is linear to the number of trap checks in the function.
Especially for functions with high numbers of trap checks we observe a
significant slowdown in compilation time, down to 0.22 MiB/s in the sqlite
benchmark instead of the average of 3 MiB/s in other benchmarks. By introducing
a TrapIf common operator only a single node is necessary per trap check, in
addition to the trap condition. Also the nodes which are shared between trap
checks (merge, effect_phi, 2 phis) would disappear. First measurements suggest a
speedup of 30-50% on average.

This CL only implements TrapIf and TrapUnless on x64. The implementation is also
hidden behind the --wasm-trap-if flag.

Please take a special look at how the source position is transfered from the
instruction selector to the code generator, and at the context that is used for
the runtime call.

R=titzer@chromium.org

Review-Url: https://codereview.chromium.org/2562393002
Cr-Commit-Position: refs/heads/master@{#41720}
Committed: https://chromium.googlesource.com/v8/v8/+/7bd61b601cc3aa637c378813f010f25e1c2f9145

[ahaas, 2016-12-12 13:51:40]

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

[commit-bot: I haz the power, 2016-12-12 13:51:52]

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-12-12 14:08:17]

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

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

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

[titzer, 2016-12-12 14:21:32]

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

[titzer, 2016-12-12 14:21:34]

+bmeurer

Looks mostly good.

https://codereview.chromium.org/2562393002/diff/1/src/compiler/instruction-se...
File src/compiler/instruction-selector-impl.h (right):

https://codereview.chromium.org/2562393002/diff/1/src/compiler/instruction-se...
src/compiler/instruction-selector-impl.h:477: int32_t trap_id_;              //
Only valid if mode_ == kFlags_trap.
I wonder if we could squeeze this into a uint8_t and whether it would therefore
fit in the alignment with the first couple of fields here.

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

https://codereview.chromium.org/2562393002/diff/1/src/compiler/wasm-compiler....
src/compiler/wasm-compiler.cc:187: #if V8_TARGET_ARCH_X64
Can you introduce another preprocessor definition, like WASM_TRAP_IF_SUPPORTED,
so that we can just change that macro as each new platform gets added (and
eventually remove)?

https://codereview.chromium.org/2562393002/diff/1/src/compiler/wasm-compiler....
src/compiler/wasm-compiler.cc:197: AddTrapIf(reason, cond, true, position);
Can we rename this internal function to BuildTrapIf() since they build an actual
subgraph as opposed to using the special TrapIf operator?

https://codereview.chromium.org/2562393002/diff/1/src/compiler/x64/code-gener...
File src/compiler/x64/code-generator-x64.cc (right):

https://codereview.chromium.org/2562393002/diff/1/src/compiler/x64/code-gener...
src/compiler/x64/code-generator-x64.cc:2313: if (frame_elided_) {
We don't need the frame teardown, since the call to the runtime should never
return. (Although we do need it in the testing case).

https://codereview.chromium.org/2562393002/diff/1/src/compiler/x64/code-gener...
src/compiler/x64/code-generator-x64.cc:2323: if (trap_id ==
Runtime::kNumFunctions) {
You should probably leave a comment here that this is for testing.

https://codereview.chromium.org/2562393002/diff/1/src/compiler/x64/code-gener...
src/compiler/x64/code-generator-x64.cc:2330:
gen_->AssembleSourcePosition(instr_);
Should the source position recording be after the call? (or maybe it doesn't
matter in that the PC is not actually used?)

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

https://codereview.chromium.org/2562393002/diff/1/src/runtime/runtime-wasm.cc...
src/runtime/runtime-wasm.cc:128: RUNTIME_FUNCTION(Runtime_ThrowWasm##name) {    
                            \
Can you factor the common code out of this macro into a helper function so that
we don't have N copies of it?

https://codereview.chromium.org/2562393002/diff/1/src/runtime/runtime.h
File src/runtime/runtime.h (right):

https://codereview.chromium.org/2562393002/diff/1/src/runtime/runtime.h#newco...
src/runtime/runtime.h:936: F(ThrowWasmTrapUnreachable, 0, 1)          \
"ThrowWasmTrap" is kind of an odd phrase.
What about "ThrowWasmXXXError"?

[ahaas, 2016-12-12 15:40:42]

On 2016/12/12 at 14:21:34, titzer wrote:
> +bmeurer
> 
> Looks mostly good.
> 
>
https://codereview.chromium.org/2562393002/diff/1/src/compiler/instruction-se...
> File src/compiler/instruction-selector-impl.h (right):
> 
>
https://codereview.chromium.org/2562393002/diff/1/src/compiler/instruction-se...
> src/compiler/instruction-selector-impl.h:477: int32_t trap_id_;             
// Only valid if mode_ == kFlags_trap.
> I wonder if we could squeeze this into a uint8_t and whether it would
therefore fit in the alignment with the first couple of fields here.
> 
>
https://codereview.chromium.org/2562393002/diff/1/src/compiler/wasm-compiler.cc
> File src/compiler/wasm-compiler.cc (right):
> 
>
https://codereview.chromium.org/2562393002/diff/1/src/compiler/wasm-compiler....
> src/compiler/wasm-compiler.cc:187: #if V8_TARGET_ARCH_X64
> Can you introduce another preprocessor definition, like
WASM_TRAP_IF_SUPPORTED, so that we can just change that macro as each new
platform gets added (and eventually remove)?
> 
>
https://codereview.chromium.org/2562393002/diff/1/src/compiler/wasm-compiler....
> src/compiler/wasm-compiler.cc:197: AddTrapIf(reason, cond, true, position);
> Can we rename this internal function to BuildTrapIf() since they build an
actual subgraph as opposed to using the special TrapIf operator?
> 
>
https://codereview.chromium.org/2562393002/diff/1/src/compiler/x64/code-gener...
> File src/compiler/x64/code-generator-x64.cc (right):
> 
>
https://codereview.chromium.org/2562393002/diff/1/src/compiler/x64/code-gener...
> src/compiler/x64/code-generator-x64.cc:2313: if (frame_elided_) {
> We don't need the frame teardown, since the call to the runtime should never
return. (Although we do need it in the testing case).
> 
>
https://codereview.chromium.org/2562393002/diff/1/src/compiler/x64/code-gener...
> src/compiler/x64/code-generator-x64.cc:2323: if (trap_id ==
Runtime::kNumFunctions) {
> You should probably leave a comment here that this is for testing.
> 
>
https://codereview.chromium.org/2562393002/diff/1/src/compiler/x64/code-gener...
> src/compiler/x64/code-generator-x64.cc:2330:
gen_->AssembleSourcePosition(instr_);
> Should the source position recording be after the call? (or maybe it doesn't
matter in that the PC is not actually used?)
> 
> https://codereview.chromium.org/2562393002/diff/1/src/runtime/runtime-wasm.cc
> File src/runtime/runtime-wasm.cc (right):
> 
>
https://codereview.chromium.org/2562393002/diff/1/src/runtime/runtime-wasm.cc...
> src/runtime/runtime-wasm.cc:128: RUNTIME_FUNCTION(Runtime_ThrowWasm##name) {  
                              \
> Can you factor the common code out of this macro into a helper function so
that we don't have N copies of it?
> 
> https://codereview.chromium.org/2562393002/diff/1/src/runtime/runtime.h
> File src/runtime/runtime.h (right):
> 
>
https://codereview.chromium.org/2562393002/diff/1/src/runtime/runtime.h#newco...
> src/runtime/runtime.h:936: F(ThrowWasmTrapUnreachable, 0, 1)          \
> "ThrowWasmTrap" is kind of an odd phrase.
> What about "ThrowWasmXXXError"?

The test-run-wasm-64/RunWasmCompiledWithTrapIf_I64RemS_Trap failed because the
diamond to check for x % -1 was not chained properly to the control chain.

[ahaas, 2016-12-13 12:38:43]

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

[commit-bot: I haz the power, 2016-12-13 12:38:53]

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

[ahaas, 2016-12-13 12:38:59]

https://codereview.chromium.org/2562393002/diff/1/src/compiler/instruction-se...
File src/compiler/instruction-selector-impl.h (right):

https://codereview.chromium.org/2562393002/diff/1/src/compiler/instruction-se...
src/compiler/instruction-selector-impl.h:477: int32_t trap_id_;              //
Only valid if mode_ == kFlags_trap.
On 2016/12/12 at 14:21:33, titzer wrote:
> I wonder if we could squeeze this into a uint8_t and whether it would
therefore fit in the alignment with the first couple of fields here.

trap_id_ contains a runtime::FunctionId, and there are more than 256 runtime
functions. Should we store the basic block pointers and trap_id +
source_position in a union so that they use the same memory? These values should
not be used together anyways.

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

https://codereview.chromium.org/2562393002/diff/1/src/compiler/wasm-compiler....
src/compiler/wasm-compiler.cc:187: #if V8_TARGET_ARCH_X64
On 2016/12/12 at 14:21:33, titzer wrote:
> Can you introduce another preprocessor definition, like
WASM_TRAP_IF_SUPPORTED, so that we can just change that macro as each new
platform gets added (and eventually remove)?

Done.

https://codereview.chromium.org/2562393002/diff/1/src/compiler/wasm-compiler....
src/compiler/wasm-compiler.cc:197: AddTrapIf(reason, cond, true, position);
On 2016/12/12 at 14:21:33, titzer wrote:
> Can we rename this internal function to BuildTrapIf() since they build an
actual subgraph as opposed to using the special TrapIf operator?

Done.

https://codereview.chromium.org/2562393002/diff/1/src/compiler/x64/code-gener...
File src/compiler/x64/code-generator-x64.cc (right):

https://codereview.chromium.org/2562393002/diff/1/src/compiler/x64/code-gener...
src/compiler/x64/code-generator-x64.cc:2330:
gen_->AssembleSourcePosition(instr_);
On 2016/12/12 at 14:21:33, titzer wrote:
> Should the source position recording be after the call? (or maybe it doesn't
matter in that the PC is not actually used?)

I looked up other code, and there it is recorded before the call. In addition,
some tests fail if it is recorded after the call.

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

https://codereview.chromium.org/2562393002/diff/1/src/runtime/runtime-wasm.cc...
src/runtime/runtime-wasm.cc:128: RUNTIME_FUNCTION(Runtime_ThrowWasm##name) {    
                            \
On 2016/12/12 at 14:21:34, titzer wrote:
> Can you factor the common code out of this macro into a helper function so
that we don't have N copies of it?

Done.

https://codereview.chromium.org/2562393002/diff/1/src/runtime/runtime.h
File src/runtime/runtime.h (right):

https://codereview.chromium.org/2562393002/diff/1/src/runtime/runtime.h#newco...
src/runtime/runtime.h:936: F(ThrowWasmTrapUnreachable, 0, 1)          \
On 2016/12/12 at 14:21:34, titzer wrote:
> "ThrowWasmTrap" is kind of an odd phrase.
> What about "ThrowWasmXXXError"?

In the macro in wasm-opcodes.h the trap names have a "Trap" prefix, like
"TrapDivByZero". Renaming would mean that I either have to change the macro, or
that I cannot use the macro.

[commit-bot: I haz the power, 2016-12-13 13:26:07]

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

[commit-bot: I haz the power, 2016-12-13 13:26:08]

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

[ahaas, 2016-12-13 14:20:43]

There is still a problem when allocating the exception object causes a GC. There
does not exist a ReferenceMap for the stack frame I create in the OOL code, and
the GC does not like that.

[ahaas, 2016-12-13 14:51:25]

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

[commit-bot: I haz the power, 2016-12-13 14:51:29]

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-12-13 15:27:51]

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

[commit-bot: I haz the power, 2016-12-13 15:27:52]

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

[ahaas, 2016-12-13 15:55:58]

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

[commit-bot: I haz the power, 2016-12-13 15:56: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, 2016-12-13 16:35:58]

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

[commit-bot: I haz the power, 2016-12-13 16:35:58]

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

[titzer, 2016-12-14 10:15:04]

lgtm

https://codereview.chromium.org/2562393002/diff/60001/src/compiler/wasm-compi...
File src/compiler/wasm-compiler.cc (right):

https://codereview.chromium.org/2562393002/diff/60001/src/compiler/wasm-compi...
src/compiler/wasm-compiler.cc:200: #endif  // V8_TARGET_ARCH_X64
End comment is out of date

[Benedikt Meurer, 2016-12-14 10:34:40]

bmeurer@chromium.org changed reviewers:
+ jarin@chromium.org

[Benedikt Meurer, 2016-12-14 10:34:41]

bmeurer@chromium.org changed required reviewers:
+ jarin@chromium.org

[Benedikt Meurer, 2016-12-14 10:34:42]

Definitely something that Jaro has to review.

https://codereview.chromium.org/2562393002/diff/60001/src/compiler/x64/code-g...
File src/compiler/x64/code-generator-x64.cc (right):

https://codereview.chromium.org/2562393002/diff/60001/src/compiler/x64/code-g...
src/compiler/x64/code-generator-x64.cc:2229: static Condition
ToCC(FlagsCondition condition) {
No static please, put it into anonymous namespace instead. Also how about
renaming to FlagsConditionToCondition? (probably applies to all architectures)

https://codereview.chromium.org/2562393002/diff/60001/src/compiler/x64/code-g...
src/compiler/x64/code-generator-x64.cc:2270: UNREACHABLE();
Nit: Move the UNREACHABLE() out of the default and nuke all the redundant
breaks. (probably applies to all archs)

https://codereview.chromium.org/2562393002/diff/60001/src/compiler/x64/code-g...
src/compiler/x64/code-generator-x64.cc:2330: if (trap_id ==
Runtime::kNumFunctions) {
This looks a bit brittle to me. Test code behaving differently than production
code has been a major issue in the past years. Can we try to find a better way
to test the actual code below? I.e. don't add the test to cctest if that's too
limited?

[titzer, 2016-12-14 10:42:39]

On 2016/12/14 10:34:42, Benedikt Meurer wrote:
> Definitely something that Jaro has to review.
> 
>
https://codereview.chromium.org/2562393002/diff/60001/src/compiler/x64/code-g...
> File src/compiler/x64/code-generator-x64.cc (right):
> 
>
https://codereview.chromium.org/2562393002/diff/60001/src/compiler/x64/code-g...
> src/compiler/x64/code-generator-x64.cc:2229: static Condition
> ToCC(FlagsCondition condition) {
> No static please, put it into anonymous namespace instead. Also how about
> renaming to FlagsConditionToCondition? (probably applies to all architectures)
> 
>
https://codereview.chromium.org/2562393002/diff/60001/src/compiler/x64/code-g...
> src/compiler/x64/code-generator-x64.cc:2270: UNREACHABLE();
> Nit: Move the UNREACHABLE() out of the default and nuke all the redundant
> breaks. (probably applies to all archs)
> 
>
https://codereview.chromium.org/2562393002/diff/60001/src/compiler/x64/code-g...
> src/compiler/x64/code-generator-x64.cc:2330: if (trap_id ==
> Runtime::kNumFunctions) {
> This looks a bit brittle to me. Test code behaving differently than production
> code has been a major issue in the past years. Can we try to find a better way
> to test the actual code below? I.e. don't add the test to cctest if that's too
> limited?

We had a long discussion with mstarzinger@ about this. We do have tests for the
entire pathway from JS, into WASM, trapping, creating the error, and getting
back out.
The cctests are more targeted just to the code generator. E.g. in the cctests we
want to test many out of bounds conditions under very careful circumstances, and
it'd be best if we didn't have to spin up a whole JS context for that. We
discussed a number of possibilities and this was the least ugliest.

[ahaas, 2016-12-14 13:48:03]

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

[commit-bot: I haz the power, 2016-12-14 13:48:10]

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

[ahaas, 2016-12-14 13:52:03]

I added an ud2 instruction behind the --debug-code flag after the runtime call
to make sure that we do not return from the runtime call. 

I also extracted the calling code in wasm-run-utils to avoid stack corruption
with setjmp/longjmp.

https://codereview.chromium.org/2562393002/diff/60001/src/compiler/x64/code-g...
File src/compiler/x64/code-generator-x64.cc (right):

https://codereview.chromium.org/2562393002/diff/60001/src/compiler/x64/code-g...
src/compiler/x64/code-generator-x64.cc:2229: static Condition
ToCC(FlagsCondition condition) {
On 2016/12/14 at 10:34:41, Benedikt Meurer wrote:
> No static please, put it into anonymous namespace instead. Also how about
renaming to FlagsConditionToCondition? (probably applies to all architectures)

Done.

https://codereview.chromium.org/2562393002/diff/60001/src/compiler/x64/code-g...
src/compiler/x64/code-generator-x64.cc:2270: UNREACHABLE();
On 2016/12/14 at 10:34:41, Benedikt Meurer wrote:
> Nit: Move the UNREACHABLE() out of the default and nuke all the redundant
breaks. (probably applies to all archs)

Done.

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

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

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

Dry run: This issue passed the CQ dry run.

[ahaas, 2016-12-14 17:48:49]

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

[commit-bot: I haz the power, 2016-12-14 17:48:51]

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-12-14 18:15:19]

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

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

Dry run: This issue passed the CQ dry run.

[ahaas, 2016-12-15 08:11:27]

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

[commit-bot: I haz the power, 2016-12-15 08:11:30]

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

[ahaas, 2016-12-15 08:12:23]

On 2016/12/14 at 13:52:03, ahaas wrote:
> I added an ud2 instruction behind the --debug-code flag after the runtime call
to make sure that we do not return from the runtime call. 
> 
> I also extracted the calling code in wasm-run-utils to avoid stack corruption
with setjmp/longjmp.
> 
>
https://codereview.chromium.org/2562393002/diff/60001/src/compiler/x64/code-g...
> File src/compiler/x64/code-generator-x64.cc (right):
> 
>
https://codereview.chromium.org/2562393002/diff/60001/src/compiler/x64/code-g...
> src/compiler/x64/code-generator-x64.cc:2229: static Condition
ToCC(FlagsCondition condition) {
> On 2016/12/14 at 10:34:41, Benedikt Meurer wrote:
> > No static please, put it into anonymous namespace instead. Also how about
renaming to FlagsConditionToCondition? (probably applies to all architectures)
> 
> Done.
> 
>
https://codereview.chromium.org/2562393002/diff/60001/src/compiler/x64/code-g...
> src/compiler/x64/code-generator-x64.cc:2270: UNREACHABLE();
> On 2016/12/14 at 10:34:41, Benedikt Meurer wrote:
> > Nit: Move the UNREACHABLE() out of the default and nuke all the redundant
breaks. (probably applies to all archs)
> 
> Done.

I added test/mjsunit/wasm/trap-location-with-trap-if.js so that there is an
mjsunit test that tests trap-if on the try bots.

[commit-bot: I haz the power, 2016-12-15 09:00:40]

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

[commit-bot: I haz the power, 2016-12-15 09:00:41]

Dry run: This issue passed the CQ dry run.

[Jarin, 2016-12-15 09:06:30]

To be honest, I am not too familiar with this area of
instruction-selector/code-gen. To my knowledge, it lgtm.

https://codereview.chromium.org/2562393002/diff/140001/src/compiler/x64/instr...
File src/compiler/x64/instruction-selector-x64.cc (right):

https://codereview.chromium.org/2562393002/diff/140001/src/compiler/x64/instr...
src/compiler/x64/instruction-selector-x64.cc:1699:
selector->SetSourcePosition(instr, *cont->source_position());
It is strange that the general source position mechanism from
InstructionSelector::VisitBlock does not kick in. Any idea why? Or are the other
cases here broken, too?

[ahaas, 2016-12-15 10:56:06]

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

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

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-12-15 11:24:55]

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

[commit-bot: I haz the power, 2016-12-15 11:24:56]

Dry run: This issue passed the CQ dry run.

[ahaas, 2016-12-15 11:31:50]

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

[commit-bot: I haz the power, 2016-12-15 11:31:59]

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

[ahaas, 2016-12-15 11:35:18]

https://codereview.chromium.org/2562393002/diff/140001/src/compiler/x64/instr...
File src/compiler/x64/instruction-selector-x64.cc (right):

https://codereview.chromium.org/2562393002/diff/140001/src/compiler/x64/instr...
src/compiler/x64/instruction-selector-x64.cc:1699:
selector->SetSourcePosition(instr, *cont->source_position());
On 2016/12/15 at 09:06:30, Jarin wrote:
> It is strange that the general source position mechanism from
InstructionSelector::VisitBlock does not kick in. Any idea why? Or are the other
cases here broken, too?

The problem with source positions is that source positions are only collected
for call nodes. I changed the code in instruction-selector.cc now so that also
source positions are collected for TrapIf and TrapUnless nodes. Thereby I can
remove all the source position handling here in the platform-specific
instruction selector.

[commit-bot: I haz the power, 2016-12-15 11:59:14]

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

[commit-bot: I haz the power, 2016-12-15 11:59:17]

Dry run: This issue passed the CQ dry run.

[ahaas, 2016-12-15 13:29:23]

The CQ bit was checked by ahaas@chromium.org

[ahaas, 2016-12-15 13:29:24]

The patchset sent to the CQ was uploaded after l-g-t-m from titzer@chromium.org,
jarin@chromium.org
Link to the patchset: https://codereview.chromium.org/2562393002/#ps180001
(title: "Rename UseSourcePosition to IsSourcePositionUsed")

[commit-bot: I haz the power, 2016-12-15 13:29:30]

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-12-15 13:31:31]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1481808563799400,
"parent_rev": "bcb38979f732f282f52876c3412b99bb48d02fcc", "commit_rev":
"7bd61b601cc3aa637c378813f010f25e1c2f9145"}

[commit-bot: I haz the power, 2016-12-15 13:31:35]

Description was changed from

==========
[wasm] Introduce the TrapIf and TrapUnless operators to generate trap code.

Some instructions in WebAssembly trap for some inputs, which means that the
execution is terminated and (at least at the moment) a JavaScript exception is
thrown. Examples for traps are out-of-bounds memory accesses, or integer
divisions by zero.

Without the TrapIf and TrapUnless operators trap check in WebAssembly introduces
5
TurboFan nodes (branch, if_true, if_false, trap-reason constant, trap-position
constant), in addition to the trap condition itself. Additionally, each
WebAssembly function has four TurboFan nodes (merge, effect_phi, 2 phis) whose
number of inputs is linear to the number of trap checks in the function.
Especially for functions with high numbers of trap checks we observe a
significant slowdown in compilation time, down to 0.22 MiB/s in the sqlite
benchmark instead of the average of 3 MiB/s in other benchmarks. By introducing
a TrapIf common operator only a single node is necessary per trap check, in
addition to the trap condition. Also the nodes which are shared between trap
checks (merge, effect_phi, 2 phis) would disappear. First measurements suggest a
speedup of 30-50% on average.

This CL only implements TrapIf and TrapUnless on x64. The implementation is also
hidden behind the --wasm-trap-if flag.

Please take a special look at how the source position is transfered from the
instruction selector to the code generator, and at the context that is used for
the runtime call.

R=titzer@chromium.org
==========

to

==========
[wasm] Introduce the TrapIf and TrapUnless operators to generate trap code.

Some instructions in WebAssembly trap for some inputs, which means that the
execution is terminated and (at least at the moment) a JavaScript exception is
thrown. Examples for traps are out-of-bounds memory accesses, or integer
divisions by zero.

Without the TrapIf and TrapUnless operators trap check in WebAssembly introduces
5
TurboFan nodes (branch, if_true, if_false, trap-reason constant, trap-position
constant), in addition to the trap condition itself. Additionally, each
WebAssembly function has four TurboFan nodes (merge, effect_phi, 2 phis) whose
number of inputs is linear to the number of trap checks in the function.
Especially for functions with high numbers of trap checks we observe a
significant slowdown in compilation time, down to 0.22 MiB/s in the sqlite
benchmark instead of the average of 3 MiB/s in other benchmarks. By introducing
a TrapIf common operator only a single node is necessary per trap check, in
addition to the trap condition. Also the nodes which are shared between trap
checks (merge, effect_phi, 2 phis) would disappear. First measurements suggest a
speedup of 30-50% on average.

This CL only implements TrapIf and TrapUnless on x64. The implementation is also
hidden behind the --wasm-trap-if flag.

Please take a special look at how the source position is transfered from the
instruction selector to the code generator, and at the context that is used for
the runtime call.

R=titzer@chromium.org

Review-Url: https://codereview.chromium.org/2562393002
Cr-Commit-Position: refs/heads/master@{#41720}
Committed:
https://chromium.googlesource.com/v8/v8/+/7bd61b601cc3aa637c378813f010f25e1c2...
==========

[commit-bot: I haz the power, 2016-12-15 13:31:36]

Committed patchset #10 (id:180001) as
https://chromium.googlesource.com/v8/v8/+/7bd61b601cc3aa637c378813f010f25e1c2...

[Michael Achenbach, 2016-12-18 21:44:31]

Note, this causes these ubsan failures:
https://build.chromium.org/p/client.v8/builders/V8%20Linux64%20-%20cfi/builds...