Closed Bug 1277008 Opened 8 years ago Closed 8 years ago

Wasm baseline: x86 support

Categories

(Core :: JavaScript Engine: JIT, defect)

Product:
Core
Component:
JavaScript Engine: JIT
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla50
Tracking Flags:
Tracking Status
firefox50 --- fixed

People

(Reporter: lth, Assigned: lth)

References

Details

Attachments

(2 files, 5 obsolete files)

bug1277008-scratchregs-v2.patch
16.29 KB, patch
luke
: review+
Details | Diff | Splinter Review
bug1277008-baseline-x86-v4.patch
29.36 KB, patch
lth
: review+
Details | Diff | Splinter Review 
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.
Attached patch bug1277008-baseline-x86.patch (obsolete) — — Splinter Review 
This passes all asm.js and wasm tests, but I'll keep it as a WIP until the baseline compiler has landed.
Assignee: nobody → lhansen
Status: NEW → ASSIGNED
No longer blocks: 1232205
Depends on: 1232205
Attached patch bug1277008-baseline-x86-v2.patch (obsolete) — — Splinter Review 
Updated to current reality; holding until the baseline jit lands.
Attachment #8758614 - Attachment is obsolete: true
Depends on: 1280927
Attached patch bug1277008-scratchregs.patch (obsolete) — — Splinter Review 
Clean up the handling of scratch registers, to account for platform variations.
Attachment #8760854 - Attachment is obsolete: true
Attached patch bug1277008-baseline-x86.patch (obsolete) — — Splinter Review 
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.
Attachment #8764934 - Flags: review?(bbouvier)
Attachment #8764935 - Flags: review?(bbouvier)
Extended and with some bug fixes.
Attachment #8764934 - Attachment is obsolete: true
Attachment #8764934 - Flags: review?(bbouvier)
Attachment #8765272 - Flags: review?(bbouvier)
Comment on attachment 8765272 [details] [diff] [review]
bug1277008-scratchregs-v2.patch

Redirecting review to offload Benjamin.
Attachment #8765272 - Flags: review?(bbouvier) → review?(luke)
Attachment #8765272 - Flags: review?(luke) → review+
Keywords: leave-open
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?
Attachment #8764935 - Flags: review?(bbouvier)
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
Flags: needinfo?(lhansen)
Annoying -- the error does not show up in local builds using the stock OS X compiler.
Flags: needinfo?(lhansen)
(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.
Attached patch bug1277008-baseline-x86-v3.patch (obsolete) — — Splinter Review 
Cleaner x86 patch (updated for review comments and with some hindsight from the ARM patch).  Not asking for review yet, pending discussions.
Attachment #8765272 - Attachment is obsolete: true
Attachment #8765272 - Attachment is obsolete: false
Attachment #8764935 - Attachment is obsolete: true
Attachment #8767622 - Flags: review?(bbouvier)
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
Attachment #8767622 - Flags: review?(bbouvier) → review+
(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+.
Attachment #8767622 - Attachment is obsolete: true
Attachment #8767851 - Flags: review+
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.
Sigh, whack-a-mole...
Flags: needinfo?(lhansen)
Needed another fix for function pointer calls.
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/4bf97306f6c3
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: