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

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Lars T Hansen [:lth]]

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.

Comment 1

[Lars T Hansen [:lth]]

Attachment: 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.

Comment 2

[Lars T Hansen [:lth]]

Oh, and the failure on simulator is for default HWCAPS, ie, ARMv7.  So not ARMv6 specific.

Comment 3

[Lars T Hansen [:lth]]

The failure goes away with --ion-osr=off (but is impervious to changes to loop unrolling, register allocation).

Comment 4

[Lars T Hansen [:lth]]

Attachment: 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.

Comment 5

[Lars T Hansen [:lth]]

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.

Comment 6

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: simd-check-int32-value.patch

Thank you for the test case! A quick search for .isInt32() showed that replaceLane is also affected.

Comment 7

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: simd-int32.patch

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).

Comment 8

[Benjamin Bouvier [:bbouvier] (inactive)]

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+.

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8cb6f0f174c1

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8cb6f0f174c1