Bug 179446 – [JSC][MIPS] Use fcsr to check the validity of the result of trunc.w.d

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Guillaume Emont]

The trunc.w.d mips instruction should give a 0x7fffffff result when the source value is Infinity, NaN, or rounds to an integer outside the range -2^31 to 2^31 -1. This is what branchTruncateDoubleToInt32() and branchTruncateDoubleToUInt32() have been relying on. It turns out that this assumption is not true on some CPUs, including on the ci20 on which we run the testbot (we get 0x80000000 instead). We should the invalid operation cause bit instead to check whether the source value could be properly truncated. This requires the addition of the cfc1 instruction, as well as the special registers that can be used with it (control registers of CP1).

[Guillaume Emont]

Created attachment 326373 [details]
Patch

The patch.

[Guillaume Emont]

Created attachment 326375 [details]
Test program to check the behavior of trunc.w.d

To help me figure out what trunc.w.d does, other than using gdb on JIT'ed code, I created this little program.
On one MIPS device it outputs expected values:

trunc.w.d of 0(0): 0    invalid operation cause bit: 0  invalid operation flag: 0
trunc.w.d of 42(0x4045000000000000): 0x2a       invalid operation cause bit: 0  invalid operation flag: 0
trunc.w.d of -1234(0xc093480000000000): 0xfffffb2e      invalid operation cause bit: 0  invalid operation flag: 0
trunc.w.d of inf(0x7ff0000000000000): 0x7fffffff        invalid operation cause bit: 1  invalid operation flag: 1
trunc.w.d of -inf(0xfff0000000000000): 0x7fffffff       invalid operation cause bit: 1  invalid operation flag: 1
trunc.w.d of nan(0x7ff7ffffffffffff): 0x7fffffff        invalid operation cause bit: 1  invalid operation flag: 1
trunc.w.d of nan(0x7fffffffffffffff): 0x7fffffff        invalid operation cause bit: 1  invalid operation flag: 1
trunc.w.d of 1.79769e+308(0x7fefffffffffffff): 0x7fffffff       invalid operation cause bit: 1  invalid operation flag: 1
trunc.w.d of -1.79769e+308(0xffefffffffffffff): 0x7fffffff      invalid operation cause bit: 1  invalid operation flag: 1
trunc.w.d of 42(0x4045000000000000): 0x2a       invalid operation cause bit: 0  invalid operation flag: 1

Though on the ci20, we get 0x80000000 for -inf and big negative values:

trunc.w.d of 0(0): 0    invalid operation cause bit: 0  invalid operation flag: 0
trunc.w.d of 42(0x4045000000000000): 0x2a       invalid operation cause bit: 0  invalid operation flag: 0
trunc.w.d of -1234(0xc093480000000000): 0xfffffb2e      invalid operation cause bit: 0  invalid operation flag: 0
trunc.w.d of inf(0x7ff0000000000000): 0x7fffffff        invalid operation cause bit: 1  invalid operation flag: 1
trunc.w.d of -inf(0xfff0000000000000): 0x80000000       invalid operation cause bit: 1  invalid operation flag: 1
trunc.w.d of nan(0x7ff7ffffffffffff): 0x7fffffff        invalid operation cause bit: 1  invalid operation flag: 1
trunc.w.d of nan(0x7fffffffffffffff): 0x7fffffff        invalid operation cause bit: 1  invalid operation flag: 1
trunc.w.d of 1.79769e+308(0x7fefffffffffffff): 0x7fffffff       invalid operation cause bit: 1  invalid operation flag: 1
trunc.w.d of -1.79769e+308(0xffefffffffffffff): 0x80000000      invalid operation cause bit: 1  invalid operation flag: 1
trunc.w.d of 42(0x4045000000000000): 0x2a       invalid operation cause bit: 0  invalid operation flag: 1

[WebKit Commit Bot]

Comment on attachment 326373 [details]
Patch

Clearing flags on attachment: 326373

Committed r224623: <https://trac.webkit.org/changeset/224623>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Adrian Perez]

Comment on attachment 326373 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=326373&action=review

Informally reviewing...

Patch looks good to me as well. There is a small issue where control registers other
than “fcsr” are _not_ available for CPUs older than MIPS V, and we may want to add
some code to handle those for WTF_MIPS_ISA<5. Adding this is not strictly needed for
this one patch (as it only uses “fcsr”) but we may want to add the suggested bits
in a follow-up patch for the sake of correcness.

> Source/JavaScriptCore/assembler/MIPSAssembler.h:116
> +    fcsr = 31

Only “fcsr” is available before MIPS V, according to the manual:

  “MIPS V and MIPS32 introduced the three control registers that access portions
   of FCSR. These registers were not available in MIPS I, II, III, or IV”

The definition for “fir” should be guarded with WTF_MIPS_ISA_AT_LEAST(5).
We may want to have some additional code to always use “fcsr” and emulate
the behavior of the other register (except “fir”) by adding a few extra
instructions to apply bit masks. Read below for more.

> Source/JavaScriptCore/assembler/MIPSAssembler.h:190
> +        case MIPSRegisters::fir:

Ditto, for ISA<5, “fir” should be guarded.

> Source/JavaScriptCore/assembler/MIPSAssembler.h:741
> +        emitInst(0x44400000 | (rt << OP_SH_RT) | (fs << OP_SH_FS));

So here we would do as it is for MIPS V and newer, and add a guard for to
handle the case with ISA<5. Something like the following:

   #if WTF_MIPS_ISA_AT_LEAST(5)
       emitInst(0x44400000 | (rt << OP_SH_RT) | (fs << OP_SH_FS));
   #else
       emitInst(0x44400000 | (rt << OP_SH_RT) | (MIPSRegisters::fcsr << OP_SH_FS));
       switch (fs) {
       case MIPSRegisters::fccr:
           andi(rt, rt, FCSR_FCCR_BITMASK);
           break;
       case MIPSRegisters::fexr:
           andi(rt, rt, FCSR_FEXR_BITMASK);
           break;
       case MIPSRegisters::fenr:
           andi(rt, rt, FCSR_FENR_BITMASK);
           break;
       case MIPSRegisters::fcsr:
           // No mask needed.
           break;
       default:
           RELEASE_ASSERT_NOT_REACHED();
       }
   #endif

Of course with the *_BITMASK constants defined accordingly.

[Adrian Perez]

Also, shouldn't we rather be checking “fcsr” as well for other *.w.[sd]
conversion instructions? (like ceil.*.*, cvt.*.*, floor.*.*, and round.*.*)
From a quick look, that would affect “branchConvertDoubleToInt32()” (which
uses the cvt.*.* instructions).

[Yusuke Suzuki]

I suggest adding a test for this in assembler/testmasm.cpp.

[Guillaume Emont]

> 
> So here we would do as it is for MIPS V and newer, and add a guard for to
> handle the case with ISA<5. Something like the following:
> 
>    #if WTF_MIPS_ISA_AT_LEAST(5)
>        emitInst(0x44400000 | (rt << OP_SH_RT) | (fs << OP_SH_FS));
>    #else
>        emitInst(0x44400000 | (rt << OP_SH_RT) | (MIPSRegisters::fcsr <<
> OP_SH_FS));
>        switch (fs) {
>        case MIPSRegisters::fccr:
>            andi(rt, rt, FCSR_FCCR_BITMASK);
>            break;
That would be a little more complicated, as the bits in fccr are not in the same position as the equivalent bits in fcsr.
In any case, another consideration is whether we want to keep on supporting anything older than mips32. I personally only have mips32 devices to test things, our testbot is mips32, and I don't know whether anyone is interested in running WebKit on older generations.

[Guillaume Emont]

(In reply to Yusuke Suzuki from comment #7)
> I suggest adding a test for this in assembler/testmasm.cpp.

Does testmasm.cpp require MASM_PROBE? We don't have that on MIPS yet (I started working on an implementation but it's on the backburner for now).

[Guillaume Emont]

(In reply to Adrian Perez from comment #6)
> Also, shouldn't we rather be checking “fcsr” as well for other *.w.[sd]
> conversion instructions? (like ceil.*.*, cvt.*.*, floor.*.*, and round.*.*)
> From a quick look, that would affect “branchConvertDoubleToInt32()” (which
> uses the cvt.*.* instructions).

We currently don't use ceil, floor or round. I will have a look at our use of cvt.*.* though.

[Mark Lam]

(In reply to Guillaume Emont from comment #9)
> (In reply to Yusuke Suzuki from comment #7)
> > I suggest adding a test for this in assembler/testmasm.cpp.
> 
> Does testmasm.cpp require MASM_PROBE? We don't have that on MIPS yet (I
> started working on an implementation but it's on the backburner for now).

No.  testmasm.cpp does not require MASM_PROBE.  It's a place to put MacroAssembler tests.  The MASM_PROBE stuff is there conditional on ENABLE(MASM_PROBE).

[Guillaume Emont]

(In reply to Adrian Perez from comment #6)
> Also, shouldn't we rather be checking “fcsr” as well for other *.w.[sd]
> conversion instructions? (like ceil.*.*, cvt.*.*, floor.*.*, and round.*.*)
> From a quick look, that would affect “branchConvertDoubleToInt32()” (which
> uses the cvt.*.* instructions).

After looking at the code, I think that branchConvertDoubleToInt32() is the only similar case, and it looks much safer as we don't rely on the return value to tell us that something went wrong in a similar way.
I wrote some tests in testmasm to make sure of that and they pass. Not sure it's worth upstreaming these tests though, especially since they are somewhat platform-specific (a platform can decide to fail (i.e. branch) for specific values it does not handle well, and I suspect this can differ from platform to platform).

[Guillaume Emont]

(In reply to Yusuke Suzuki from comment #7)
> I suggest adding a test for this in assembler/testmasm.cpp.

I did add tests, and should have done it earlier: they made me realize of some mistakes in my code. I filed a separate bug 179563 and posted a patch fixing it, this time with testmasm tests provide.d

[Radar WebKit Bug Importer]

<rdar://problem/35562137>