Summary: | Implement all the arithmetic and logical instructions in WebAssembly | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Sukolsak Sakshuwong <sukolsak> | ||||||
Component: | JavaScriptCore | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | commit-queue, fpizlo, ggaren, mark.lam, saam, sukolsak, ysuzuki | ||||||
Priority: | P2 | ||||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Bug Depends on: | 148838 | ||||||||
Bug Blocks: | 146064 | ||||||||
Attachments: |
|
Description
Sukolsak Sakshuwong
2015-09-04 21:50:23 PDT
Created attachment 260675 [details]
Patch
Created attachment 260730 [details]
Patch
Comment on attachment 260730 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=260730&action=review r=me > Source/JavaScriptCore/tests/stress/wasm-arithmetic.js:45 > + return (6 * 7) | 0; What's stopping the WASM compiler from just constant folding this operation and just returning a constant (thereby defeating this test)? Why not pass the values into the test functions as arguments? Unless there's a reason to not do so, please pass the args in to the test instead. I'm fine with doing a follow up patch to update all the tests to do this. > Source/JavaScriptCore/tests/stress/wasm-arithmetic.js:192 > +shouldBe(module.multiplyOverflow(), -2147483648); What does it mean to multiplyOverflow? Does WASM require that we overflow silently? (In reply to comment #3) > Comment on attachment 260730 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=260730&action=review > > r=me > > > Source/JavaScriptCore/tests/stress/wasm-arithmetic.js:45 > > + return (6 * 7) | 0; > > What's stopping the WASM compiler from just constant folding this operation > and just returning a constant (thereby defeating this test)? Why not pass > the values into the test functions as arguments? Unless there's a reason to > not do so, please pass the args in to the test instead. I'm fine with doing > a follow up patch to update all the tests to do this. pack-asmjs does not do constant folding. (I don't think they will ever do it. Optimization of asm.js should be done by Emscripten's.) But I will do it to be future-proof. It should also make the code easier to read. > > Source/JavaScriptCore/tests/stress/wasm-arithmetic.js:192 > > +shouldBe(module.multiplyOverflow(), -2147483648); > > What does it mean to multiplyOverflow? Does WASM require that we overflow > silently? multiplyOverflow = the result from multiplication is larger than INT32_MAX. The WASM spec says "Sign-agnostic operations silently wrap overflowing results into the result type." It also says that multiplication on 32-bit integers is a sign-agnostic operation. <https://github.com/WebAssembly/design/blob/master/AstSemantics.md> (In reply to comment #4) > (In reply to comment #3) > > Comment on attachment 260730 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=260730&action=review > > > > r=me > > > > > Source/JavaScriptCore/tests/stress/wasm-arithmetic.js:45 > > > + return (6 * 7) | 0; > > > > What's stopping the WASM compiler from just constant folding this operation > > and just returning a constant (thereby defeating this test)? Why not pass > > the values into the test functions as arguments? Unless there's a reason to > > not do so, please pass the args in to the test instead. I'm fine with doing > > a follow up patch to update all the tests to do this. > > pack-asmjs does not do constant folding. (I don't think they will ever do > it. Optimization of asm.js should be done by Emscripten's.) But I will do it > to be future-proof. It should also make the code easier to read. OK. Let's do this in a separate patch since there are other tests (than the ones in this patch) that would benefit from this refactoring as well. > > > Source/JavaScriptCore/tests/stress/wasm-arithmetic.js:192 > > > +shouldBe(module.multiplyOverflow(), -2147483648); > > > > What does it mean to multiplyOverflow? Does WASM require that we overflow > > silently? > > multiplyOverflow = the result from multiplication is larger than INT32_MAX. > The WASM spec says "Sign-agnostic operations silently wrap overflowing > results into the result type." It also says that multiplication on 32-bit > integers is a sign-agnostic operation. > <https://github.com/WebAssembly/design/blob/master/AstSemantics.md> OK. Comment on attachment 260730 [details] Patch Clearing flags on attachment: 260730 Committed r189499: <http://trac.webkit.org/changeset/189499> All reviewed patches have been landed. Closing bug. |