Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(541)

Issue 2584603002: [wasm] TrapIf and TrapUnless TurboFan operators implemented on arm. (Closed)

Created:
4 years ago by ahaas
Modified:
4 years ago
Reviewers:
titzer, georgia.kouveli, v8-arm-ports
CC:
v8-mips-ports_googlegroups.com, v8-ppc-ports_googlegroups.com, v8-reviews_googlegroups.com
Target Ref:
refs/pending/heads/master
Project:
v8
Visibility:
Public.

Description

[wasm] TrapIf and TrapUnless TurboFan operators implemented on arm. Original commit message: [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, v8-arm-ports@googlegroups.com Review-Url: https://codereview.chromium.org/2584603002 Cr-Commit-Position: refs/heads/master@{#41774} Committed: https://chromium.googlesource.com/v8/v8/+/ca8d3ba7183d2b7a9691b2e732f7dbe8dc719a3e

Patch Set 1 #

Total comments: 2

Patch Set 2 : Removed obsolete DCHECKs #

Patch Set 3 : rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+98 lines, -11 lines) Patch
M src/bailout-reason.h View 1 chunk +3 lines, -1 line 0 comments Download
M src/compiler/arm/code-generator-arm.cc View 1 chunk +58 lines, -1 line 0 comments Download
M src/compiler/arm/instruction-selector-arm.cc View 1 7 chunks +36 lines, -8 lines 0 comments Download
M src/compiler/wasm-compiler.cc View 1 2 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 19 (13 generated)
ahaas
4 years ago (2016-12-15 15:20:23 UTC) #1
georgia.kouveli
Just a very minor comment. https://codereview.chromium.org/2584603002/diff/1/src/compiler/arm/instruction-selector-arm.cc File src/compiler/arm/instruction-selector-arm.cc (right): https://codereview.chromium.org/2584603002/diff/1/src/compiler/arm/instruction-selector-arm.cc#newcode1267 src/compiler/arm/instruction-selector-arm.cc:1267: DCHECK(cont->IsSet()); You've missed a ...
4 years ago (2016-12-15 16:39:35 UTC) #7
ahaas
https://codereview.chromium.org/2584603002/diff/1/src/compiler/arm/instruction-selector-arm.cc File src/compiler/arm/instruction-selector-arm.cc (right): https://codereview.chromium.org/2584603002/diff/1/src/compiler/arm/instruction-selector-arm.cc#newcode1267 src/compiler/arm/instruction-selector-arm.cc:1267: DCHECK(cont->IsSet()); On 2016/12/15 at 16:39:35, georgia.kouveli wrote: > You've ...
4 years ago (2016-12-15 16:57:59 UTC) #10
titzer
On 2016/12/15 16:57:59, ahaas wrote: > https://codereview.chromium.org/2584603002/diff/1/src/compiler/arm/instruction-selector-arm.cc > File src/compiler/arm/instruction-selector-arm.cc (right): > > https://codereview.chromium.org/2584603002/diff/1/src/compiler/arm/instruction-selector-arm.cc#newcode1267 > ...
4 years ago (2016-12-16 09:08:16 UTC) #13
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2584603002/40001
4 years ago (2016-12-16 16:46:31 UTC) #16
commit-bot: I haz the power
4 years ago (2016-12-16 17:16:51 UTC) #19
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/v8/v8/+/ca8d3ba7183d2b7a9691b2e732f7dbe8dc7...

Powered by Google App Engine
This is Rietveld 408576698