Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1702293002:
Add asm.js stdlib portion implementable as wasm opcodes. (Closed)
|
Created:
4 years, 10 months ago by bradnelson Modified:
4 years, 10 months ago CC:
v8-reviews_googlegroups.com Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
DescriptionAdd asm.js stdlib portion implementable as wasm opcodes.
Lost in the repo shuffle:
https://github.com/WebAssembly/v8-native-prototype/pull/102
BUG= https://code.google.com/p/v8/issues/detail?id=4203
TEST=mjsunit/asm-wasm
R=aseemgarg@chromium.org,titzer@chromium.org
LOG=N
Committed: https://crrev.com/af903021c6d6f7fbd186d1fb82e434e9156d2e50
Cr-Commit-Position: refs/heads/master@{#34218}
Patch Set 1 #Patch Set 2 : !./to #Patch Set 3 : reflow #
Total comments: 10
Patch Set 4 : fixes #Patch Set 5 : fix win #
Total comments: 10
Patch Set 6 : fixes #
Total comments: 2
Messages
Total messages: 25 (9 generated)
The CQ bit was checked by bradnelson@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1702293002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1702293002/20001
The CQ bit was checked by bradnelson@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1702293002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1702293002/40001
Lost in the repo shuffle, was LGed, but PTAL as I should be asleep now...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_win_nosnap_shared_compile_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_compil...)
https://codereview.chromium.org/1702293002/diff/40001/src/wasm/asm-wasm-build... File src/wasm/asm-wasm-builder.cc (right): https://codereview.chromium.org/1702293002/diff/40001/src/wasm/asm-wasm-build... src/wasm/asm-wasm-builder.cc:928: current_function_builder_->Emit(kExprF32Min); Can you double check that the JS min/max correspond exactly to the WASM min/max? I seem to recall there might be a difference. https://codereview.chromium.org/1702293002/diff/40001/src/wasm/asm-wasm-build... src/wasm/asm-wasm-builder.cc:955: case AsmTyper::kMathImul: { Should be a simple kExprI32Mul.
lgtm after taking care of Ben's comments
ignore lgtm. Want to discuss the part of code to deal with functions too. https://codereview.chromium.org/1702293002/diff/40001/src/wasm/asm-wasm-build... File src/wasm/asm-wasm-builder.cc (right): https://codereview.chromium.org/1702293002/diff/40001/src/wasm/asm-wasm-build... src/wasm/asm-wasm-builder.cc:448: return; get rid of the return and instead make the condition for var->IsContextSlot an "else if". https://codereview.chromium.org/1702293002/diff/40001/src/wasm/asm-wasm-build... src/wasm/asm-wasm-builder.cc:669: if (vp != nullptr && vp->var()->IsParameter()) { what's the change here? https://codereview.chromium.org/1702293002/diff/40001/src/wasm/asm-wasm-build... src/wasm/asm-wasm-builder.cc:670: fprintf(stderr, "%d\n", vp->var()->index()); get rid of fprintf
Description was changed from ========== Add asm.js stdlib portion implemented as wasm opcodes. Lost in the repo shuffle: https://github.com/WebAssembly/v8-native-prototype/pull/102 BUG= https://code.google.com/p/v8/issues/detail?id=4203 TEST=mjsunit/asm-wasm R=aseemgarg@chromium.org,titzer@chromium.org LOG=N ========== to ========== Add asm.js stdlib portion implementable as wasm opcodes. Lost in the repo shuffle: https://github.com/WebAssembly/v8-native-prototype/pull/102 BUG= https://code.google.com/p/v8/issues/detail?id=4203 TEST=mjsunit/asm-wasm R=aseemgarg@chromium.org,titzer@chromium.org LOG=N ==========
bradnelson@google.com changed reviewers: + bradnelson@google.com
PTAL https://codereview.chromium.org/1702293002/diff/40001/src/wasm/asm-wasm-build... File src/wasm/asm-wasm-builder.cc (right): https://codereview.chromium.org/1702293002/diff/40001/src/wasm/asm-wasm-build... src/wasm/asm-wasm-builder.cc:448: return; On 2016/02/17 19:44:06, aseemgarg wrote: > get rid of the return and instead make the condition for var->IsContextSlot an > "else if". But I'm returning because I don't actually want to get the GetLocal/Global below this. https://codereview.chromium.org/1702293002/diff/40001/src/wasm/asm-wasm-build... src/wasm/asm-wasm-builder.cc:669: if (vp != nullptr && vp->var()->IsParameter()) { On 2016/02/17 19:44:06, aseemgarg wrote: > what's the change here? Oops, dropped. https://codereview.chromium.org/1702293002/diff/40001/src/wasm/asm-wasm-build... src/wasm/asm-wasm-builder.cc:670: fprintf(stderr, "%d\n", vp->var()->index()); On 2016/02/17 19:44:06, aseemgarg wrote: > get rid of fprintf Done. https://codereview.chromium.org/1702293002/diff/40001/src/wasm/asm-wasm-build... src/wasm/asm-wasm-builder.cc:928: current_function_builder_->Emit(kExprF32Min); On 2016/02/17 09:49:33, titzer wrote: > Can you double check that the JS min/max correspond exactly to the WASM min/max? > I seem to recall there might be a difference. Yes there is a difference around NaNs I intend to gate the behavior for these opcodes on it being an asm module. But will defer to a separate change. Added a TODO. https://codereview.chromium.org/1702293002/diff/40001/src/wasm/asm-wasm-build... src/wasm/asm-wasm-builder.cc:955: case AsmTyper::kMathImul: { On 2016/02/17 09:49:33, titzer wrote: > Should be a simple kExprI32Mul. Done.
Ping? I have a change stacked on top to route the other Math methods thru external calls...
Please see inline comments https://codereview.chromium.org/1702293002/diff/80001/src/wasm/asm-wasm-build... File src/wasm/asm-wasm-builder.cc (right): https://codereview.chromium.org/1702293002/diff/80001/src/wasm/asm-wasm-build... src/wasm/asm-wasm-builder.cc:388: current_function_builder_->EmitCode(code, sizeof(code)); move the EmitCode and return to outside of cases and use breaks instead. seems more conventional way of writing switch. https://codereview.chromium.org/1702293002/diff/80001/src/wasm/asm-wasm-build... src/wasm/asm-wasm-builder.cc:663: Property* prop = expr->value()->AsProperty(); This doesn't seem quite right. Stdlib constants would show up as binary operations. So, these checks should be part of the if above. https://codereview.chromium.org/1702293002/diff/80001/src/wasm/asm-wasm-build... src/wasm/asm-wasm-builder.cc:980: // Visit arguments. The visiting of args should be done in the visit call it self. This function should emit the opcode and then rely on common code in visitCall to deal with args. https://codereview.chromium.org/1702293002/diff/80001/test/mjsunit/wasm/asm-w... File test/mjsunit/wasm/asm-wasm.js (right): https://codereview.chromium.org/1702293002/diff/80001/test/mjsunit/wasm/asm-w... test/mjsunit/wasm/asm-wasm.js:1287: // TODO(bradnelson): Figure out why this fails. Could this be some parser magic not implemented correctly? Maybe try 1.0/0.0 https://codereview.chromium.org/1702293002/diff/80001/test/mjsunit/wasm/asm-w... test/mjsunit/wasm/asm-wasm.js:1333: if (StdlibMathSqrt(123.0) != 11.090536506409418) return 0; Change it to a perfect square? That way reading the test is little easier..
PTAL https://codereview.chromium.org/1702293002/diff/80001/src/wasm/asm-wasm-build... File src/wasm/asm-wasm-builder.cc (right): https://codereview.chromium.org/1702293002/diff/80001/src/wasm/asm-wasm-build... src/wasm/asm-wasm-builder.cc:388: current_function_builder_->EmitCode(code, sizeof(code)); On 2016/02/22 22:15:53, aseemgarg wrote: > move the EmitCode and return to outside of cases and use breaks instead. seems > more conventional way of writing switch. Done. https://codereview.chromium.org/1702293002/diff/80001/src/wasm/asm-wasm-build... src/wasm/asm-wasm-builder.cc:663: Property* prop = expr->value()->AsProperty(); On 2016/02/22 22:15:53, aseemgarg wrote: > This doesn't seem quite right. Stdlib constants would show up as binary > operations. So, these checks should be part of the if above. So unlike foreign globals, stdlib functions come in without a type declaration: var pi = stdlib.Math.PI; So this is skipping these, since there's no init code to emit. Updated comment. Also I've refactored to merge with the item below for clarity. https://codereview.chromium.org/1702293002/diff/80001/src/wasm/asm-wasm-build... src/wasm/asm-wasm-builder.cc:980: // Visit arguments. On 2016/02/22 22:15:53, aseemgarg wrote: > The visiting of args should be done in the visit call it self. This function > should emit the opcode and then rely on common code in visitCall to deal with > args. So sharing the later call is tricky, as Fround wants to be able to pick out literals as a special case. I've refactored calling the arguments into a shared method. https://codereview.chromium.org/1702293002/diff/80001/test/mjsunit/wasm/asm-w... File test/mjsunit/wasm/asm-wasm.js (right): https://codereview.chromium.org/1702293002/diff/80001/test/mjsunit/wasm/asm-w... test/mjsunit/wasm/asm-wasm.js:1287: // TODO(bradnelson): Figure out why this fails. On 2016/02/22 22:15:53, aseemgarg wrote: > Could this be some parser magic not implemented correctly? Maybe try 1.0/0.0 Not thinking. All comparisons on NaN's fail. Changed to a separate function that checks with isNaN. https://codereview.chromium.org/1702293002/diff/80001/test/mjsunit/wasm/asm-w... test/mjsunit/wasm/asm-wasm.js:1333: if (StdlibMathSqrt(123.0) != 11.090536506409418) return 0; On 2016/02/22 22:15:53, aseemgarg wrote: > Change it to a perfect square? That way reading the test is little easier.. Done.
lgtm https://codereview.chromium.org/1702293002/diff/100001/src/wasm/asm-wasm-buil... File src/wasm/asm-wasm-builder.cc (right): https://codereview.chromium.org/1702293002/diff/100001/src/wasm/asm-wasm-buil... src/wasm/asm-wasm-builder.cc:919: // TODO(bradnelson): Change wasm to match Math.min in asm.js mode. Are we collecting tests for these somewhere?
https://codereview.chromium.org/1702293002/diff/100001/src/wasm/asm-wasm-buil... File src/wasm/asm-wasm-builder.cc (right): https://codereview.chromium.org/1702293002/diff/100001/src/wasm/asm-wasm-buil... src/wasm/asm-wasm-builder.cc:919: // TODO(bradnelson): Change wasm to match Math.min in asm.js mode. On 2016/02/23 15:57:58, titzer wrote: > Are we collecting tests for these somewhere? Will do.
The CQ bit was checked by bradnelson@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from aseemgarg@chromium.org Link to the patchset: https://codereview.chromium.org/1702293002/#ps100001 (title: "fixes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1702293002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1702293002/100001
Message was sent while issue was closed.
Description was changed from ========== Add asm.js stdlib portion implementable as wasm opcodes. Lost in the repo shuffle: https://github.com/WebAssembly/v8-native-prototype/pull/102 BUG= https://code.google.com/p/v8/issues/detail?id=4203 TEST=mjsunit/asm-wasm R=aseemgarg@chromium.org,titzer@chromium.org LOG=N ========== to ========== Add asm.js stdlib portion implementable as wasm opcodes. Lost in the repo shuffle: https://github.com/WebAssembly/v8-native-prototype/pull/102 BUG= https://code.google.com/p/v8/issues/detail?id=4203 TEST=mjsunit/asm-wasm R=aseemgarg@chromium.org,titzer@chromium.org LOG=N ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Add asm.js stdlib portion implementable as wasm opcodes. Lost in the repo shuffle: https://github.com/WebAssembly/v8-native-prototype/pull/102 BUG= https://code.google.com/p/v8/issues/detail?id=4203 TEST=mjsunit/asm-wasm R=aseemgarg@chromium.org,titzer@chromium.org LOG=N ========== to ========== Add asm.js stdlib portion implementable as wasm opcodes. Lost in the repo shuffle: https://github.com/WebAssembly/v8-native-prototype/pull/102 BUG= https://code.google.com/p/v8/issues/detail?id=4203 TEST=mjsunit/asm-wasm R=aseemgarg@chromium.org,titzer@chromium.org LOG=N Committed: https://crrev.com/af903021c6d6f7fbd186d1fb82e434e9156d2e50 Cr-Commit-Position: refs/heads/master@{#34218} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/af903021c6d6f7fbd186d1fb82e434e9156d2e50 Cr-Commit-Position: refs/heads/master@{#34218} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
