There are a few of the platform hooks in the initial wasm baseline compiler (bug 1232205) that are x64-only that have to be in place before we have functional parity on x86. (The biggest chunk is range-checked heap accesses.) This bug is the placeholder for those fixes.

This passes all asm.js and wasm tests, but I'll keep it as a WIP until the baseline compiler has landed.

Updated to current reality; holding until the baseline jit lands.

Clean up the handling of scratch registers, to account for platform variations.

Remaining bits for x86, almost all range checking code.

Comment on attachment 8764934 [details] [diff] [review] bug1277008-scratchregs.patch This patch sits on top of the unreachable-code patch in bug 1280927.

Extended and with some bug fixes.

Comment on attachment 8765272 [details] [diff] [review] bug1277008-scratchregs-v2.patch Redirecting review to offload Benjamin.

https://hg.mozilla.org/integration/mozilla-inbound/rev/a36da0eea7af1bdb7e51b69d5aa9454b98689ac3 Bug 1277008 - Clean up scratch register management. r=luke

Comment on attachment 8764935 [details] [diff] [review] bug1277008-baseline-x86.patch Review of attachment 8764935 [details] [diff] [review]: ----------------------------------------------------------------- I'm very concerned by all the copy/pasto from the CodeGeneratorX86Shared into this file: as soon as e.g,. the wasm load/store land, I am pretty sure this code will be wrong (for instance, because after the wasm load/store, we don't use the x86 instruction JA but we use JAE). And I'm also pretty sure there already was a refactoring making this code look different in CodeGeneratorX86Shared. Do you have time to update this patch to share more code before the SAB work? Otherwise I'll review as is and somebody can take care of the refactoring later. Other than that, great work! I don't see any good reason to not give an r+, if not for this refactoring. ::: js/src/asmjs/WasmBaselineCompile.cpp @@ +308,5 @@ > +#endif > + case NONE: > + MOZ_CRASH("AnyReg::any() on NONE"); > + default: > + return AnyRegister(); Can you remove the default? You've enumerated all the values above, and adding a new value here will trigger a compiler warning rather than an assertion later (you can also MOZ_CRASH outside the switch, as each case returns). @@ +3151,5 @@ > + bool emitBoundsCheckBranch(const MAsmJSHeapAccess& access, RegI32 ptr, Label* maybeFail) { > + Label* pass = nullptr; > + > + if (needsOffsetBoundsCheck(access)) { > + auto oolCheck = new(alloc_) OffsetBoundsCheck(maybeFail, ptr.reg, access.offset()); nit: auto* @@ +3300,5 @@ > + switch (access.accessType()) { > + case Scalar::Int8: > + case Scalar::Uint8: { > + Register rd = dest.i32().reg; > + Register tx = rd; What does `tx` mean? @@ +3324,1 @@ > uint32_t after = masm.size(); It seems this should go in the x64 branch with the call to verifyHeapAccessDisassembly. @@ +3364,5 @@ > +#elif defined(JS_CODEGEN_X86) > + Operand dstAddr(ptr.reg, access.offset()); > + > + bool didMove = false; > + if ((access.accessType() == Scalar::Int8 || access.accessType() == Scalar::Uint8) && access.size() == 1 is more compact @@ +3391,1 @@ > uint32_t after = masm.size(); same remark as for the load. @@ +6778,5 @@ > specific_ecx(RegI32(ecx)), > specific_edx(RegI32(edx)), > #endif > +#if defined(JS_CODEGEN_X86) > + singleByteRegs_(GeneralRegisterSet(Registers::SingleByteRegs)), It seems this is always unchanged. Could it be a static const global attribute, instead?

Backed out for warning as error in WasmBaselineCompile.cpp on OSX: https://hg.mozilla.org/integration/mozilla-inbound/rev/fdadd7ef691e Push with failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=a36da0eea7af1bdb7e51b69d5aa9454b98689ac3 Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=31075230&repo=mozilla-inbound 09:10:25 INFO - /builds/slave/m-in-m64-000000000000000000000/build/src/js/src/asmjs/WasmBaselineCompile.cpp:206:23: error: private field 'bc' is not used [-Werror,-Wunused-private-field] 09:10:25 INFO - BaseCompiler& bc; 09:10:25 INFO - ^ 09:10:25 INFO - 1 error generated. 09:10:25 INFO - make[6]: *** [Unified_cpp_js_src0.o] Error 1

Annoying -- the error does not show up in local builds using the stock OS X compiler.

(In reply to Benjamin Bouvier [:bbouvier] from comment #9) > Comment on attachment 8764935 [details] [diff] [review] > bug1277008-baseline-x86.patch > > Review of attachment 8764935 [details] [diff] [review]: > ----------------------------------------------------------------- > > I'm very concerned by all the copy/pasto from the CodeGeneratorX86Shared > into this file: as soon as e.g,. the wasm load/store land, I am pretty sure > this code will be wrong (for instance, because after the wasm load/store, we > don't use the x86 instruction JA but we use JAE). And I'm also pretty sure > there already was a refactoring making this code look different in > CodeGeneratorX86Shared. > > Do you have time to update this patch to share more code before the SAB > work? Otherwise I'll review as is and somebody can take care of the > refactoring later. Let's discuss this on IRC... > ::: js/src/asmjs/WasmBaselineCompile.cpp > @@ +308,5 @@ > > +#endif > > + case NONE: > > + MOZ_CRASH("AnyReg::any() on NONE"); > > + default: > > + return AnyRegister(); > > Can you remove the default? You've enumerated all the values above, and > adding a new value here will trigger a compiler warning rather than an > assertion later (you can also MOZ_CRASH outside the switch, as each case > returns). You can't win: Removing the default makes gcc complain about a branch without a proper return value. (gcc5 on several platforms.) But sure, I can MOZ_CRASH outside the switch.

Cleaner x86 patch (updated for review comments and with some hindsight from the ARM patch). Not asking for review yet, pending discussions.

https://hg.mozilla.org/integration/mozilla-inbound/rev/7ad5763a7404c420b93301f17ea2e9bb66e5f00b Bug 1277008 - Clean up scratch register management. r=luke

Comment on attachment 8767622 [details] [diff] [review] bug1277008-baseline-x86-v3.patch Review of attachment 8767622 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me, thank you for the patch. Let's open a follow-up bug for the refactoring to be done later; in the meanwhile, this is good to have in-tree. ::: js/src/asmjs/WasmBaselineCompile.cpp @@ +2775,5 @@ > + > + return access.needsBoundsCheck(); > + } > + > + bool faultOnOutOfBounds(const MWasmMemoryAccess& access) { OK, it took me some time to realize, while reading the uses of this function, that this didn't have anything to deal with x64 faulting, as in SIGSEGV being sent to the program. This is totally orthogonal, but the name confused me. Can we rename to "throwOnOutOfBounds", or anything that would remove a possible confusion with signal handling? (I initially thought about "trapOnOutOfBounds", but "trap" could also be confusing) @@ +2811,5 @@ > + int32_t offset; > + > + public: > + OffsetBoundsCheck(Label* maybeOutOfBounds, Register ptrReg, int32_t offset) > + : maybeOutOfBounds(maybeOutOfBounds), nit: 2 spaces before the ":" @@ +2882,5 @@ > + Scalar::Type viewType; > + AnyRegister dest; > + public: > + OutOfLineLoadTypedArrayOOB(Scalar::Type viewType, AnyRegister dest) > + : viewType(viewType), ditto @@ +2994,5 @@ > + switch (access.accessType()) { > + case Scalar::Int8: > + case Scalar::Uint8: { > + Register rd = dest.i32().reg; > + Register rt = rd; (this makes it sound like it's ARM code :-)) @@ +2998,5 @@ > + Register rt = rd; > + if (!SingleByteRegs.has(rd)) { > + mustMove = true; > + rt = ScratchRegX86; > + } For what it's worth, I prefer the way it's done in the store function: checking for byteSize() == 1 above the switch, and keeping the int8/uint8 cases tidy. @@ -2784,4 @@ > uint32_t after = masm.size(); > - > - masm.append(AsmJSMemoryAccess(before, wasm::MemoryAccess::CarryOn)); > - verifyHeapAccessDisassembly(before, after, IsLoad(true), access.accessType(), 0, srcAddr, dest); Can you keep this for the x64 code? We still disassemble the instruction in the signal handler, so it is nice to be able to check this after emitting the load/store. @@ -2815,4 @@ > uint32_t after = masm.size(); > > - masm.append(AsmJSMemoryAccess(before, wasm::MemoryAccess::CarryOn)); > - verifyHeapAccessDisassembly(before, after, IsLoad(false), access.accessType(), 0, dstAddr, src); ditto

(In reply to Benjamin Bouvier [:bbouvier] from comment #9) > Comment on attachment 8764935 [details] [diff] [review] > bug1277008-baseline-x86.patch > > > @@ +6778,5 @@ > > specific_ecx(RegI32(ecx)), > > specific_edx(RegI32(edx)), > > #endif > > +#if defined(JS_CODEGEN_X86) > > + singleByteRegs_(GeneralRegisterSet(Registers::SingleByteRegs)), > > It seems this is always unchanged. Could it be a static const global > attribute, instead? That's a good idea, but MSVC miscompiles the code at least on 32-bit (it thinks the variable definition looks like a function definition), so I've reverted to what we have here, for the time being.

(In reply to Lars T Hansen [:lth] from comment #19) > https://treeherder.mozilla.org/#/jobs?repo=try&revision=55897b8fc907 Fails some asm.js CGC and P tests with --wasm-always-baseline on Windows XP (for the absolute maximum amount of joy). The CGC failure is probably a red herring; there are simply some tests that don't pass. No info in the tests results, but ABI issues would be one place to start.

Patch with all remarks addressed, but with the WinXP failure. Carrying bbouvier's r+.

BTW, these are the failures: TEST-UNEXPECTED-FAIL | js\src\jit-test\tests\asm.js\simd-mandelbrot.js | Unknown (code -2147483645, args "--wasm-always-baseline") TEST-UNEXPECTED-FAIL | js\src\jit-test\tests\asm.js\testCloning.js | Unknown (code -2147483645, args "--wasm-always-baseline") TEST-UNEXPECTED-FAIL | js\src\jit-test\tests\asm.js\testFFI.js | Unknown (code -2147483645, args "--wasm-always-baseline") TEST-UNEXPECTED-FAIL | js\src\jit-test\tests\asm.js\testFunctionPtr.js | Unknown (code -2147483645, args "--wasm-always-baseline") TEST-UNEXPECTED-FAIL | js\src\jit-test\tests\asm.js\testStackWalking.js | Unknown (code -2147483645, args "--wasm-always-baseline")

A problem with stack alignment, these are all INT 3 that are hit because the stack is not properly aligned on ffi calls. Why this happens only on WinXP...

WasmBaselineCompile is using ABIStackAlignment but with !__GNUC__ this is too small for WasmStubs. Should use JitStackAlignment everywhere.

And with that fixed there are some range checking subtleties that were not handled properly.

That try run has some infra issues for sure but otherwise looks OK.

https://hg.mozilla.org/integration/mozilla-inbound/rev/dcb7835534e21a62cebdd5d1205d4e81f6bc6946 Bug 1277008 - Wasm baseline: x86 support. r=bbouvier

This caused some jit test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=31746859&repo=mozilla-inbound Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/80e7a1b549de

Sigh, whack-a-mole...

Needed another fix for function pointer calls.

https://hg.mozilla.org/integration/mozilla-inbound/rev/4bf97306f6c321c82650dfc260b260fe0ea7c599 Bug 1277008 - Wasm baseline: x86 support. r=bbouvier