Closed Bug 1204604 Opened 6 years ago Closed 6 years ago

SIMD.swizzle C++ versions (etc) need to check for int32 values, not int32 tags

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: lth, Assigned: bbouvier)

References

Details

Attachments

(3 files, 1 obsolete file)

These tests fail:

    /home/pi/moz/js/src/jit-test/tests/SIMD/shuffle.js
    /home/pi/moz/js/src/jit-test/tests/SIMD/swizzle.js

They both fail like this:

TypeError: invalid arguments
TypeError: invalid arguments
TypeError: invalid arguments
/home/pi/moz/js/src/jit-test/tests/SIMD/swizzle.js:56:13 Error: Assertion failed: got 148, expected 149

Cross-compiled release build.  Have not been able to repro on the Simulator so far.
Attached file swizzle.js
This reduced test case reproduces on ARM simulator (debug build) with --no-threads.

This could perhaps look like an indication of broken bailout code on ARM (stale value of "i" being picked up?) but that's just guessing.
Oh, and the failure on simulator is for default HWCAPS, ie, ARMv7.  So not ARMv6 specific.
Summary: ARMv6: SIMD failures on Raspberry Pi 1 → SIMD failures on bailout tests
The failure goes away with --ion-osr=off (but is impervious to changes to loop unrolling, register allocation).
Summary: SIMD failures on bailout tests → SIMD failures on bailout tests - OSR implicated
Attached file swizzle.js, v2
A variation on the same test case.  The loop bounds don't matter as long as they're large enough; it is the value of ion.warmup.trigger that matters.  Increase this and the point at which we fail - the value of "i" in the loop - increases by the same amount.

When the cutoff is 50 then the exception is thrown on the 148th iteration.
When the cutoff is 100 then the exception is thrown on the 198th iteration.

The generated ion code looks OK to me so this probably has to do with how we're transitioning into ion code after the compilation and OSR.
The reason for the failure is probably in the C++ implementation of swizzle.

That code tests explicitly for an int32 representation for its arguments.  In the test program, the caller has an argument expression that looks like this:

  condition ? 2.5 : 1

That works fine when it is called from the interpreter or baseline code: these will push a tagged double or a tagged int, and the swizzle code will fail when it sees 2.5, as it should.  Once we jump into jitted code it is no longer fine: the code generated for that by ion is to push either the double 2.5 or the double 1.0.  nbp says this is correct and I believe him.  But now the swizzle code will fail also for 1.0, which it should not.

Ergo the swizzle code will need to check for an int32 value, not an int32 tag, and likely there are similar errors elsewhere in the SIMD code.
Flags: needinfo?(benj)
OS: Linux → All
Hardware: ARM → All
Summary: SIMD failures on bailout tests - OSR implicated → SIMD.swizzle C++ versions (etc) need to check for int32 values, not int32 tags
Attached patch simd-check-int32-value.patch (obsolete) — Splinter Review
Thank you for the test case! A quick search for .isInt32() showed that replaceLane is also affected.
Assignee: nobody → benj
Status: NEW → ASSIGNED
Flags: needinfo?(benj)
Attachment #8662929 - Flags: review?(lhansen)
Attachment #8662929 - Flags: review?(lhansen) → review+
Attached patch simd-int32.patchSplinter Review
Hmm, this showed another error in the JIT implementation: extractLane, replaceLane, shuffle and swizzle shouldn't ever be removed, for they contain range checks on the lane inputs.

I'll file a follow-up for having a better strategy here (we can probably decouple the bounds check from the instruction itself).
Attachment #8662929 - Attachment is obsolete: true
Actually, Simd{Extract,Insert}Element are emitted only if the lane indexes are in bounds int32, so no need to make them guard instructions. However, it's needed for the SimdGeneralShuffle. Carrying forward r+.
https://hg.mozilla.org/mozilla-central/rev/8cb6f0f174c1
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.