Closed Bug 1333770 Opened 5 years ago Closed 5 years ago
Wasm baseline: Remove asm
.js support from Rabaldr
A recent change shunts the compilation of asm.js code around the baseline compiler: changeset: 329691:b353d014c22b user: Luke Wagner <firstname.lastname@example.org> date: Tue Jan 17 13:13:51 2017 -0600 summary: Bug 1330891 - Baldr: simplify ModuleGenerator (r=bbouvier) In truth, that was just cleanup from an earlier change that entirely removed the ability of the baseline compiler to compile asm.js code (because the baseline compiler needs to know whether the asm.js code uses simd or atomics and that information is not available with concurrent validation): changeset: 328334:b4d14dfa3cf8 user: David Major <email@example.com> date: Fri Jan 06 17:15:48 2017 -0600 summary: Bug 1329018 - Part 2: Wasm: Move function validation to helper threads. r=luke I don't care for that change, because it makes test coverage of the baseline compiler strictly worse, but given that the change has been made (and given that concurrent verification is a real win), we should just remove the asm-js specific code paths in the baseline compiler. Be sure to clean up code that only the asm.js paths use, such as the VolatileReturnGPR static. Let's do this for the next release (hence P1).
Luke, is it a correct assumption that we will not need asm.js support in Rabaldr going forward?
That's a good point about the raw loss of testing coverage for baseline. A moderately-simple alternative would be to add a JITOptions gate for SIMD.js (just as with asm.js+Atomics) and then have the (Atomics-gate || SIMD-gate) disable baseline+asm.js which could be done upfront at the beginning ModuleGenerator. If we can get wasm-embenchen into jit-tests as you suggested in the other bug, I think that would end up giving us a big boost in wasm test coverage so the asm.js coverage wouldn't be as valuable. But to answer your question, I don't think it's necessary for rabaldr to support asm.js.
(In reply to Luke Wagner [:luke] from comment #2) > If we can get wasm-embenchen into jit-tests as you > suggested in the other bug, I think that would end up giving us a big boost > in wasm test coverage so the asm.js coverage wouldn't be as valuable. I like that point of view, I'll make sure that gets done this cycle. > But to answer your question, I don't think it's necessary for rabaldr to support > asm.js. Then I will ask for a review on the patch on this bug :-)
Assignee: nobody → lhansen
Status: NEW → ASSIGNED
Removes all asm.js support from Rabaldr, and asserts that we're not compiling asm.js when we enter.
Comment on attachment 8830791 [details] [diff] [review] bug1333770-remove-asmjs-v2.patch Review of attachment 8830791 [details] [diff] [review]: ----------------------------------------------------------------- Mmm, that's nice. ::: js/src/wasm/WasmBaselineCompile.cpp @@ +2471,5 @@ > const SigWithId& sig = env_.sigs[sigIndex]; > > CalleeDesc callee; > + MOZ_ASSERT(sig.id.kind() != SigIdDesc::Kind::None); > + MOZ_ASSERT(env_.tables.length() == 1); super-nit: can you merge the declaration of 'callee' with the initialization below, remove the \n between the def of 'sig' and this MOZ_ASSERT() about sig.id.kind() and lastly add a \n before the MOZ_ASSERT() abut env_.tables?
Attachment #8830791 - Flags: review?(luke) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/505e39fbc5ed9857ba12c1c12608fc1cca5c43be Bug 1333770 - wasm baseline, remove dead asm.js support. r=luke
There are some bytecodes that are only used by code generated from asm.js, which Rabaldr won't see now. These bytecodes are not marked with "asm.js" in any obvious way in Rabaldr, so they were not removed by the patch on this bug. I believe the following bytecodes are all implemented: TeeGlobal, I32Min, I32Max, I32Neg, I32BitNot, I32Abs, F32TeeStoreF64, F64TeeStoreF32, I32TeeStore8, I32TeeStore16, I64TeeStore8, I64TeeStore16, I64TeeStore32, I32TeeStore, I64TeeStore, F32TeeStore, F64TeeStore, F64Mod, F64Sin, F64Cos, F64Tan, F64Asin, F64Acos, F64Atan, F64Exp, F64Log, F64Pow, F64Atan2, The question is how many of these we believe might show up in a later edition of WebAssembly. I might be inclined to just keep these for now, at most comment them out or mark them with comments. Luke, Benjamin -- opinions?
Agreed with Benjamin on removing all of these from the baseline. I don't anticipate any of these being added to wasm in the near future and I also agree that dead code is best removed eagerly.
Reopening for additional work, let's keep the P1.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to Benjamin Bouvier [:bbouvier] from comment #9) > (In reply to Lars T Hansen [:lth] from comment #8) > > F64Mod, > > F64Sin, > > F64Cos, > > F64Tan, > > F64Asin, > > F64Acos, > > F64Atan, > > F64Exp, > > F64Log, > > F64Pow, > > F64Atan2, > > ... > (there should be some f32 functions here too, iirc) There are only F64 variants in the current WasmBinaryConstants.h and the list I posted above is the full list of opcodes flagged as "asm.js", apart from atomics and simd and oldcallindirect. oldcallindirect is implemented by the baseline compiler, and goes away too, I expect.
Pretty straightforward, just remove unused code and inline templated functions that are now referenced only once. I have also closed the bug that is referenced from a comment within the removed code.
Comment on attachment 8832422 [details] [diff] [review] bug1333770-remove-asmjs-part2.patch Removing r?, there's one more thing to do.
As the previous patch, but removes additional code and comments that only applied to the now-removed teeStore. This allows us to close bug 1331264 regarding a possible optimization of that code; the optimization is now easily implemented and is part of the present patch.
Additionally emitCommonMathCall can be inlined into emitUnaryMathBuiltinCall.
Comment on attachment 8832435 [details] [diff] [review] bug1333770-remove-asmjs-part2-v2.patch Review of attachment 8832435 [details] [diff] [review]: ----------------------------------------------------------------- LGTM, thanks. ::: js/src/wasm/WasmBaselineCompile.cpp @@ +5762,5 @@ > > endCall(baselineCall, stackSpace); > > // For new style calls, the callee was popped off the compiler's > // stack above. This comment could be deleted now, or at least updated. @@ +6310,1 @@ > RegI32 tmp1 = temps >= 1 ? needI32() : invalidI32(); Drive-by remark, but it seems that if for some reason in the future temps > 1 (cause storeTemps decides to use more than 1), then we might need more than 1 register. Could we assert it's exactly 0 or 1, or explicitly fault when it's more than 1? (also, I don't get the `1` suffix in this variable name)
Attachment #8832435 - Flags: review?(bbouvier) → review+
(In reply to Benjamin Bouvier [:bbouvier] from comment #17) > Comment on attachment 8832435 [details] [diff] [review] > bug1333770-remove-asmjs-part2-v2.patch > > Review of attachment 8832435 [details] [diff] [review]: > ----------------------------------------------------------------- > > @@ +6310,1 @@ > > RegI32 tmp1 = temps >= 1 ? needI32() : invalidI32(); > > (also, I don't get the `1` suffix in this variable name) Probable personal idiosyncracy: there are other places in the code that use the same pattern with multiple temps, and this keeps the naming consistent. But an alternate explanation could be that there were multiple temps here before. Either way I think you're right, I'll clean this up...
https://hg.mozilla.org/integration/mozilla-inbound/rev/de946df35c9758b89625198086c46cf27f96dca2 Bug 1333770 - Wasm baseline, remove remaining asm.js support. r=bbouvier
Status: REOPENED → RESOLVED
Closed: 5 years ago → 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.