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.