1406999 - [MIPS32] Cleanup floating point registers handling

Status: RESOLVED FIXED

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

Description

[Dragan Mladjenovic]

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.

Comment 1

[Dragan Mladjenovic]

Attachment: bug1406999.patch

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

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

Comment 3

[Nicolas B. Pierron [:nbp]]

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 .

Comment 4

[Dragan Mladjenovic]

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

Comment 5

[Dragan Mladjenovic]

Attachment: convertValueToFloatingPoint.diff

Forces tmp to be register of Double type in order to silence the asserts on MIPS. Will be merged with main patch.

Comment 6

[Dragan Mladjenovic]

Attachment: bug1406999_2.patch

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.

Comment 7

[Dragan Mladjenovic]

Ping.

Comment 8

[Dragan Mladjenovic]

Attachment: bug1406999_3.patch

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f7837e3840d
[MIPS32] Cleanup floating point registers handling. r=nbp

Comment 10

[Narcis Beleuzu [:NarcisB]]

Backed out for build bustages on MacroAssembler.cpp

Backout rev: https://hg.mozilla.org/integration/mozilla-inbound/rev/c6297aca62a422e4a8988a9a8b186e22d737aa99
Log file:    https://treeherder.mozilla.org/logviewer.html#?job_id=152825601&repo=mozilla-inbound&lineNumber=5878

Comment 11

[Dragan Mladjenovic]

Attachment: bug1406999_4.patch

Adds FloatRegister::asDouble() for arm64 in order to fix build bustage.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e4410d944a07
[MIPS32] Cleanup floating point registers handling; r=nbp

Comment 13

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/e4410d944a07

Comment 14

[Mike Hommey [:glandium]]

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?

Comment 15

[Mike Hommey [:glandium]]

See comment 14.

Comment 16

[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 ?

Comment 17

[Mike Hommey [:glandium]]

The Debian mips port defaults to -mfpxx... it'd be sad to have to disable the JIT.

Comment 18

[Mike Hommey [:glandium]]

We only figured out now because we didn't have a rust compiler before.