Closed Bug 1406999 Opened 2 years ago Closed 2 years ago

[MIPS32] Cleanup floating point registers handling

Categories

(Core :: JavaScript Engine: JIT, enhancement, P5)

enhancement

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox57 --- wontfix
firefox58 --- wontfix
firefox59 --- fixed

People

(Reporter: dragan.mladjenovic, Assigned: dragan.mladjenovic)

Details

Attachments

(1 file, 4 obsolete files)

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/51.0.2704.106 Safari/537.36



Actual results:

Split from Bug 1329650. Simplifies the floating point register set implementation for mips32 to only include 16 even single and double precision registers. This removes the need for incomplete workarounds for Loongson3A. Also fixes MacroAssembler::storeRegsInMask implementation which was not in the line with MacroAssembler::PopRegsInMask. Changes FloatRegister::getRegisterDumpOffsetInBytes to return correct offsets for double precision registers.
Attached patch bug1406999.patch (obsolete) — Splinter Review
Attachment #8916706 - Flags: feedback?(bbouvier)
Assignee: nobody → dragan.mladjenovic
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Priority: -- → P5
Comment on attachment 8916706 [details] [diff] [review]
bug1406999.patch

Review of attachment 8916706 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the very long review time. Skimmed the patch and it looks good, but this is really nbp's territory, so redirecting request to him. Thanks for the patch!

::: js/src/jit/mips-shared/Architecture-mips-shared.h
@@ +319,3 @@
>      static uint32_t SetSize(SetType x) {
>          static_assert(sizeof(SetType) == 8, "SetType must be 64 bits");
> +        //TODO: check

Remaining todo?

@@ +350,5 @@
>  
> +
> +// MIPS64 doesn't support it and on MIPS32 we don't allocate odd single fp
> +// registers thus not exposing multi aliasing to the jit.
> +// See comments in Arhitecture-mips32.h.

nit: Architecture*

::: js/src/jit/mips32/Architecture-mips32.cpp
@@ +77,5 @@
> +    MOZ_ASSERT((bits & 0xFFFF) == 0);
> +    uint32_t ret =  mozilla::CountPopulation32(bits) * sizeof(double);
> +
> +    // Additional space needed by MacroAssembler::PushRegsInMask to ensure
> +    // correct alignment of double values.

Shouldn't it be done by PushRegsInMask, then?

::: js/src/jit/mips32/MacroAssembler-mips32.cpp
@@ +2120,5 @@
>      const int32_t reservedF = diffF;
>  
> +    if (reservedF > 0) {
> +        // Read the buffer form the first aligned location.
> +        ma_addu(SecondScratchReg, sp, Imm32(reservedF));

maybe use a ScratchRegisterScope for SecondScratchReg?

@@ +2161,5 @@
>      }
>      MOZ_ASSERT(diffG == 0);
>  
> +    if (diffF > 0) {
> +

nit: please remove blank line
Attachment #8916706 - Flags: feedback?(nicolas.b.pierron)
Attachment #8916706 - Flags: feedback?(bbouvier)
Attachment #8916706 - Flags: feedback+
Comment on attachment 8916706 [details] [diff] [review]
bug1406999.patch

Review of attachment 8916706 [details] [diff] [review]:
-----------------------------------------------------------------

I will ask more feedback about the encoding and usage and addition of the Assembler instruction.

::: js/src/jit/mips-shared/Architecture-mips-shared.h
@@ +350,5 @@
>  
> +
> +// MIPS64 doesn't support it and on MIPS32 we don't allocate odd single fp
> +// registers thus not exposing multi aliasing to the jit.
> +// See comments in Arhitecture-mips32.h.

If so, we should probably comment and fix the aliases/numAliased function from mips32/Architecture-mips32.h .
Attachment #8916706 - Flags: feedback?(r)
Attachment #8916706 - Flags: feedback?(nicolas.b.pierron)
Attachment #8916706 - Flags: feedback+
(In reply to Benjamin Bouvier [:bbouvier] from comment #2)
> Comment on attachment 8916706 [details] [diff] [review]
> bug1406999.patch
> 
> Review of attachment 8916706 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry for the very long review time. Skimmed the patch and it looks good,
> but this is really nbp's territory, so redirecting request to him. Thanks
> for the patch!
> 
> ::: js/src/jit/mips-shared/Architecture-mips-shared.h
> @@ +319,3 @@
> >      static uint32_t SetSize(SetType x) {
> >          static_assert(sizeof(SetType) == 8, "SetType must be 64 bits");
> > +        //TODO: check
> 
> Remaining todo?

Sorry, missed that during the patch squashing. Will remove it. It seems that only architecture that uses FloatRegisterSet::size() is x64 and rest of it probably have broken implementation depending of what do 
you expect for this method to return. 

> ::: js/src/jit/mips32/Architecture-mips32.cpp
> @@ +77,5 @@
> > +    MOZ_ASSERT((bits & 0xFFFF) == 0);
> > +    uint32_t ret =  mozilla::CountPopulation32(bits) * sizeof(double);
> > +
> > +    // Additional space needed by MacroAssembler::PushRegsInMask to ensure
> > +    // correct alignment of double values.
> 
> Shouldn't it be done by PushRegsInMask, then?
I've done it this way to cover these two uses. 
http://searchfox.org/mozilla-central/source/js/src/jit/IonCacheIRCompiler.cpp#207
http://searchfox.org/mozilla-central/source/js/src/wasm/WasmStubs.cpp#244
The MacroAssembler::PushRegsInMask "pushes" those 8 bytes unconditionally on the stack because is does dynamic
stack realignment. For correctness I thought it would be better to include it here than to worry about any possible 
future use of FloatRegisterSet::getPushSizeInBytes. This might change in the future. I'm thinking of auditing all uses of PushRegsInMask to see if we could relay on MacroAssembler::getFramePushed() to do static stack realignment, but that won't be part of this patch.    
   
> ::: js/src/jit/mips32/MacroAssembler-mips32.cpp
> @@ +2120,5 @@
> >      const int32_t reservedF = diffF;
> >  
> > +    if (reservedF > 0) {
> > +        // Read the buffer form the first aligned location.
> > +        ma_addu(SecondScratchReg, sp, Imm32(reservedF));
> 
> maybe use a ScratchRegisterScope for SecondScratchReg?

I've thought of doing this "one day tm" in a separate patch related to adding more uses of ScratchRegisterScope and SecondScratchRegisterScope trough MIPS code.
Attached patch convertValueToFloatingPoint.diff (obsolete) — Splinter Review
Forces tmp to be register of Double type in order to silence the asserts on MIPS. Will be merged with main patch.
Attachment #8926949 - Flags: review?(nicolas.b.pierron)
Attachment #8926949 - Flags: review?(nicolas.b.pierron) → review+
Attachment #8916706 - Flags: review?(nicolas.b.pierron)
Attached patch bug1406999_2.patch (obsolete) — Splinter Review
Small cleanup and fix to FloatRegister::alignedOrDominatedAliasedSet. The method incorrectly calculated set value for Double registers. Issue didn't manifest itself until used in wasm baseline register allocator.
Attachment #8916706 - Attachment is obsolete: true
Attachment #8916706 - Flags: review?(nicolas.b.pierron)
Attachment #8916706 - Flags: feedback?(r)
Attachment #8932412 - Flags: review?(nicolas.b.pierron)
Ping.
Attachment #8932412 - Flags: review?(nicolas.b.pierron) → review+
Attached patch bug1406999_3.patch (obsolete) — Splinter Review
Attachment #8926949 - Attachment is obsolete: true
Attachment #8932412 - Attachment is obsolete: true
Attachment #8937986 - Flags: review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f7837e3840d
[MIPS32] Cleanup floating point registers handling. r=nbp
Keywords: checkin-needed
Adds FloatRegister::asDouble() for arm64 in order to fix build bustage.
Attachment #8937986 - Attachment is obsolete: true
Flags: needinfo?(dragan.mladjenovic)
Attachment #8938293 - Flags: review+
Keywords: checkin-needed
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e4410d944a07
[MIPS32] Cleanup floating point registers handling; r=nbp
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e4410d944a07
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Comment on attachment 8938293 [details] [diff] [review]
bug1406999_4.patch

Review of attachment 8938293 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/mips32/Architecture-mips32.h
@@ +45,5 @@
> +// as double precision registers. It enables jit code to run even on Loongson3A.
> +// It does not support FR=1 mode because MacroAssembler threats odd single precision
> +// registers as high parts of even double precision registers.
> +#ifdef __mips_fpr
> +static_assert(__mips_fpr == 32, "MIPS32 jit only supports FR=0 fpu mode.");

This blocks building with -mfpxx, should this be < 64?
See comment 14.
Flags: needinfo?(dragan.mladjenovic)
Currently there are places in mips32 JIT that still assume that double floating point value in register is contained in even odd register pair (FR=0). We cannot allow it to be built with -fpxx because it will fail in "weird" ways when run on hardware that supports FP=1 mode. So it currently the Spidermonkey on mips32 needs to be built with -fp32 or w/o JIT. I haven't got around to do the JIT cleanup that would make it FR=1 safe yet.  At that time this was done intentionally to allow JIT to be used on pre mips32r2 hardware. How much of an issue is this for you ?
Flags: needinfo?(dragan.mladjenovic)
The Debian mips port defaults to -mfpxx... it'd be sad to have to disable the JIT.
We only figured out now because we didn't have a rust compiler before.
You need to log in before you can comment on or make changes to this bug.