Closed Bug 972836 Opened 6 years ago Closed 5 years ago

IonMonkey MIPS: Make use of odd Float32 registers

Categories

(Core :: JavaScript Engine: JIT, defect)

Other
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: rankov, Assigned: rankov)

References

Details

Attachments

(5 files, 10 obsolete files)

19.11 KB, patch
rankov
: review+
cbook
: checkin+
Details | Diff | Splinter Review
2.61 KB, patch
rankov
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review
8.56 KB, patch
mjrosenb
: review+
Details | Diff | Splinter Review
68.41 KB, patch
rankov
: review+
Details | Diff | Splinter Review
1.22 KB, patch
mjrosenb
: review+
Details | Diff | Splinter Review
Currently MIPS port uses only 16 even float registers (f0, f2 .. f30). These are used as both float64 and float32. 

We want to use the odd registers (f1, f3 ... f31) as float32 registers.

The problem which has to be resolved is allocation. If we use f2 as float64 register, then we can't use f2 or f3 as float32. And vice-versa: If we use f3 as float32, then we can't use f2 as float64.
Depends on: 969375
Attached patch Add-Odd-Float-Registers.patch (obsolete) — Splinter Review
Assignee: nobody → branislav.rankov
Status: NEW → ASSIGNED
CC: Heiher. Once the patch Add-Odd-Float-Registers.patch is reviewed, you can start commiting your patches to bug Bug 985881 .
Attachment #8397741 - Flags: review?(nfroyd)
Comment on attachment 8397741 [details] [diff] [review]
Add-Odd-Float-Registers.patch

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

This works for me; I would like to double-check whether Nicolas has an opinion about this.

::: js/src/jit/mips/Architecture-mips.h
@@ +199,5 @@
> +// - 64 bit floating-point coprocessor - In this case, there are 32 double
> +// precision register which can also be used as single precision registers.
> +
> +// When using O32 ABI, floating-point coprocessor is 32 bit or working in 32
> +// bit mode.

This comment is nonsensical: 32 bit or 32 bit?

::: js/src/jit/mips/MoveEmitter-mips.cpp
@@ +250,2 @@
>              if(to.reg() == a2)
>                  masm.as_mfc1(a2, from.floatReg());

It seems like you want to express this at a higher level than the raw assembler...moveFromDoubleLo?  I guess that might not be quite right for N32 or N64...
Attachment #8397741 - Flags: review?(nicolas.b.pierron)
Attachment #8397741 - Flags: review?(nfroyd)
Attachment #8397741 - Flags: review+
(In reply to Nathan Froyd (:froydnj) from comment #3)
> ::: js/src/jit/mips/Architecture-mips.h
> @@ +199,5 @@
> > +// When using O32 ABI, floating-point coprocessor is 32 bit or working in 32
> > +// bit mode.
> 
> This comment is nonsensical: 32 bit or 32 bit?

I will fix this.

> ::: js/src/jit/mips/MoveEmitter-mips.cpp
> @@ +250,2 @@
> >              if(to.reg() == a2)
> >                  masm.as_mfc1(a2, from.floatReg());
> 
> It seems like you want to express this at a higher level than the raw
> assembler...moveFromDoubleLo?  I guess that might not be quite right for N32
> or N64...

It will work on N32 and N64, but I can make a MacroAssembler function so that it looks better.
Comment on attachment 8397741 [details] [diff] [review]
Add-Odd-Float-Registers.patch

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

Sorry for the delay, this patch looks better than only having half of the registers listed :)
Attachment #8397741 - Flags: review?(nicolas.b.pierron) → review+
Carry review from previous patch.

I have fixed issues noted by Nathan in comment #3.
Attachment #8397741 - Attachment is obsolete: true
Attachment #8402710 - Flags: review+
Nicolas, can you commit this patch? 

We should leave the bug open, since the odd registers are now present, but they still need to be allocated.
Flags: needinfo?(nicolas.b.pierron)
Keywords: leave-open
Attachment #8402710 - Flags: checkin?(nicolas.b.pierron)
Flags: needinfo?(nicolas.b.pierron)
Attachment #8402710 - Flags: checkin?(nicolas.b.pierron) → checkin+
Attached patch Odd-Float-Registers-part-2.patch (obsolete) — Splinter Review
I missed this part in the previous patch.
Attachment #8411851 - Flags: review?(jdemooij)
Comment on attachment 8411851 [details] [diff] [review]
Odd-Float-Registers-part-2.patch

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

::: js/src/jit/mips/Trampoline-mips.cpp
@@ +336,5 @@
>      }
>  
>      // Save floating point registers
>      // We can use as_sd because stack is alligned.
> +    // :TODO: (Bug 972836) // Fix this once odd regs can be used as float32

Nit: remove second "//"

@@ +558,5 @@
>      }
>  
>      // Save floating point registers
>      // We can use as_sd because stack is alligned.
> +    // :TODO: (Bug 972836) // Fix this once odd regs can be used as float32

same here
Attachment #8411851 - Flags: review?(jdemooij) → review+
Carry review from previous patch. Updated r=jandem
Attachment #8411851 - Attachment is obsolete: true
Attachment #8418073 - Flags: review+
Attachment #8418073 - Flags: checkin?(nicolas.b.pierron)
Hi Branislav,

could you provide a Try link. Suggestions for what to run if you haven't
yet can be found here:
https://wiki.mozilla.org/Sheriffing/How:To:Recommended_Try_Practices

Thanks!

- Tomcat
(In reply to Carsten Book [:Tomcat] from comment #13)
> could you provide a Try link. Suggestions for what to run if you haven't
> yet can be found here:
> https://wiki.mozilla.org/Sheriffing/How:To:Recommended_Try_Practices

Here is the try link:
https://tbpl.mozilla.org/?tree=Try&rev=5964504030c3
Comment on attachment 8418073 [details] [diff] [review]
Odd-Float-Registers-part-2.patch

https://hg.mozilla.org/integration/mozilla-inbound/rev/5a98a30d97b1
Attachment #8418073 - Flags: checkin?(nicolas.b.pierron) → checkin+
Depends on: 991153
Attached patch Float32-full-support-part1.patch (obsolete) — Splinter Review
This is MIPS port for changes in the bug 991153 that have landed so far.
Attachment #8451753 - Flags: review?(mrosenberg)
Comment on attachment 8451753 [details] [diff] [review]
Float32-full-support-part1.patch

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

::: js/src/jit/mips/Architecture-mips.cpp
@@ +109,5 @@
> +            // a double with an overlay, add in both floats
> +            mod.addUnchecked((*iter).singleOverlay(0));
> +            mod.addUnchecked((*iter).singleOverlay(1));
> +        } else {
> +            // add in the lone double

Capitalize 'add'x2, and 'a', trailing periods.

@@ +120,5 @@
> +uint32_t
> +FloatRegister::GetSizeInBytes(const FloatRegisterSet &s)
> +{
> +    uint64_t bits = s.bits();
> +    uint32_t ret = mozilla::CountPopulation32(bits&0xffffffff) * sizeof(float);

Spaces around |&|

@@ +129,5 @@
> +FloatRegister::GetPushSizeInBytes(const FloatRegisterSet &s)
> +{
> +    FloatRegisterSet ss = s.reduceSetForPush();
> +    uint64_t bits = ss.bits();
> +    uint32_t ret = mozilla::CountPopulation32(bits&0xffffffff) * sizeof(float);

spaces around |&|

::: js/src/jit/mips/Architecture-mips.h
@@ +294,5 @@
>  
>      static const uint32_t WrapperMask = VolatileMask;
>  
> +    // Only create mask for double registers. Float32 registers will be
> +    // handled trough aliassing API.

It looks like you still aren't allocating odd numbered registers, but I'm not positive.

@@ +341,5 @@
> +
> +  protected:
> +    RegType kind_ : 1;
> +  public:
> +    Code code_ : 6;

Are 6 bits actually necessary? there are plenty of bits to spare, so it doesn't really matter.

@@ +429,5 @@
> +            MOZ_ASSERT((code_ & 1) == 0);
> +            return 2;
> +        }
> +        // f1-float32 has 0 other aligned aliases, 1 total.
> +        // s0-float32 has 1 other aligned alias, 2 total.

Should update s0->f0 as well, probably.

@@ +434,5 @@
> +        return 2 - (code_ & 1);
> +    }
> +    // |        f0-double        |
> +    // | f0-float32 | f1-float32 |
> +    // if we've stored f0-float32 and f1-float32 in memory, we also want to

Captialize 'if'.
Attachment #8451753 - Flags: review?(mrosenberg) → review+
(In reply to Marty Rosenberg [:mjrosenb] from comment #18)
> ::: js/src/jit/mips/Architecture-mips.h
> @@ +294,5 @@
> >  
> >      static const uint32_t WrapperMask = VolatileMask;
> >  
> > +    // Only create mask for double registers. Float32 registers will be
> > +    // handled trough aliassing API.
> 
> It looks like you still aren't allocating odd numbered registers, but I'm
> not positive.

You are right. This comment is wrong. I will remove it. I'm waiting for all the patches in bug 991153 to land. Then I will add support for odd numbered registers.

> @@ +341,5 @@
> > +
> > +  protected:
> > +    RegType kind_ : 1;
> > +  public:
> > +    Code code_ : 6;
> 
> Are 6 bits actually necessary? there are plenty of bits to spare, so it
> doesn't really matter.

I need 6 bits because I also encode invalid_freg in this field. I could have made a separate bit for invalid registers, but it would complicate code and still use 6 bits.
Attached patch Float32-full-support-part1.patch (obsolete) — Splinter Review
Fixed the comments and style issues.
Carry review from previous patch.
Attachment #8451753 - Attachment is obsolete: true
Attachment #8452992 - Flags: review+
Attached patch Float32-full-support-part1.patch (obsolete) — Splinter Review
Replacing previous patch because there were modifications on the same pieces of code.
Attachment #8452992 - Attachment is obsolete: true
Attachment #8465418 - Flags: review?(mrosenberg)
Attached patch Float32-full-support-part2.patch (obsolete) — Splinter Review
Attachment #8465420 - Flags: review?(mrosenberg)
Attached patch Float32-full-support-part2.patch (obsolete) — Splinter Review
Rebased patch because asmjs files have been moved.
Attachment #8465420 - Attachment is obsolete: true
Attachment #8465420 - Flags: review?(mrosenberg)
Attachment #8468512 - Flags: review?(mrosenberg)
Comment on attachment 8465418 [details] [diff] [review]
Float32-full-support-part1.patch

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

The volatile_ comment is the only thing I am really concerned about.
You may also want to modify js/src/jsapi-tests/testJitMoveEmitterCycles.cpp so it works on MIPS as well, since handling cycles in the move emitter is almost certainly not tested, and MIPS has a simulator, which is the requirement for that file.
If you think volatile_ is correct, just re-r? me, and it'll be a fast review.

::: js/src/jit/mips/Architecture-mips.cpp
@@ +126,5 @@
> +FloatRegister::GetPushSizeInBytes(const FloatRegisterSet &s)
> +{
> +    FloatRegisterSet ss = s.reduceSetForPush();
> +    uint64_t bits = ss.bits();
> +    uint32_t ret = mozilla::CountPopulation32(bits & 0xffffffff) * sizeof(float);

If it is an invariant that you only push the Double registers, then this should probably assert that bits & 0xffffffff == 0

::: js/src/jit/mips/Architecture-mips.h
@@ +281,5 @@
>      static Code FromName(const char *name);
>  
>      static const Code Invalid = invalid_freg;
>  
> +    static const uint32_t Total = 64;

Shouldn't total be 48 here?

@@ +371,5 @@
> +
> +  protected:
> +    RegType kind_ : 1;
> +  public:
> +    uint32_t code_ : 6;

If there are only 32 float registers total, this should be 5, no?

@@ +426,5 @@
> +
> +    bool volatile_() const {
> +        if (isDouble())
> +            return !!(code_ & FloatRegisters::VolatileMask);
> +        return !!((code_ & ~1) & FloatRegisters::VolatileMask);

This function looks very wrong.  You almost certainly want 1 << code_.

@@ +474,5 @@
> +    // | f0-float32 | f1-float32 |
> +    // If we've stored f0-float32 and f1-float32 in memory, we also want to
> +    // say that f0-double is stored there, but it is only stored at the
> +    // location where it is aligned e.g. at f0-float32, not f1-float32.
> +    void alignedAliased(uint32_t aliasIdx, FloatRegister *ret) {

FWIW, as long as you only push Double registers, you should never need to worry about the f1 case.  If there are no plans to change that, you should probably just assert that you never get an odd register.

::: js/src/jit/mips/Assembler-mips.h
@@ +110,5 @@
>  static MOZ_CONSTEXPR_VAR Register JSReturnReg_Data = a2;
>  static MOZ_CONSTEXPR_VAR Register StackPointer = sp;
>  static MOZ_CONSTEXPR_VAR Register FramePointer = InvalidReg;
>  static MOZ_CONSTEXPR_VAR Register ReturnReg = v0;
> +static MOZ_CONSTEXPR_VAR FloatRegister ReturnFloat32Reg(FloatRegisters::f0, FloatRegister::Single);

Minor nit: You may want to see if you can use the curly-brace initializers here.  I had to switch ARM to the constructor-based initializer because they didn't work, but if MIPS can avoid whatever forced me to use these initializers, that would probably be best.
Attachment #8465418 - Flags: review?(mrosenberg)
Attachment #8468512 - Flags: review?(mrosenberg)
(In reply to Marty Rosenberg [:mjrosenb] from comment #25)

> ::: js/src/jit/mips/Architecture-mips.h
> @@ +281,5 @@
> >      static Code FromName(const char *name);
> >  
> >      static const Code Invalid = invalid_freg;
> >  
> > +    static const uint32_t Total = 64;
> 
> Shouldn't total be 48 here?

This constant is used to loop trough register mask. Because there are holes in register mask, this has to be 64. Maybe we should change the name of the constant, or make another one for this purpose.

> 
> @@ +371,5 @@
> > +
> > +  protected:
> > +    RegType kind_ : 1;
> > +  public:
> > +    uint32_t code_ : 6;
> 
> If there are only 32 float registers total, this should be 5, no?

The invalid_reg value is 32. This is why I need 6 bits. I will see if I can make a separate bit for invalid register.

> 
> @@ +426,5 @@
> > +
> > +    bool volatile_() const {
> > +        if (isDouble())
> > +            return !!(code_ & FloatRegisters::VolatileMask);
> > +        return !!((code_ & ~1) & FloatRegisters::VolatileMask);
> 
> This function looks very wrong.  You almost certainly want 1 << code_.

You are right.

I will fix the rest of the issues.
Attached patch Float32-full-support-part1.patch (obsolete) — Splinter Review
I have fixed all listed issues except two:
- uint32_t code_ : 6;
- static const uint32_t Total = 64;

If you want to change these, I would prefer to make a separate patch because there would be more files touched by changes.
Attachment #8465418 - Attachment is obsolete: true
Attachment #8472974 - Flags: review?(mrosenberg)
Comment on attachment 8472974 [details] [diff] [review]
Float32-full-support-part1.patch

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

::: js/src/jit/mips/Architecture-mips.h
@@ +371,5 @@
> +        return kind_ == other.kind_ && code_ == other.code_;
> +    }
> +    bool isDouble() const { return kind_ == Double; }
> +    bool isSingle() const { return kind_ == Single; }
> +    bool isFloat() const { return (kind_ == Double) || (kind_ == Single); }

This seems like a bit of overkill, considering these registers only handle double and single values.

@@ +407,5 @@
> +    }
> +
> +    bool volatile_() const {
> +        if (isDouble())
> +            return !!((1 << code_) & FloatRegisters::VolatileMask);

1ULL

@@ +408,5 @@
> +
> +    bool volatile_() const {
> +        if (isDouble())
> +            return !!((1 << code_) & FloatRegisters::VolatileMask);
> +        return !!((1 << (code_ & ~1)) & FloatRegisters::VolatileMask);

1 is safe, 1ULL is more uniform
Attachment #8472974 - Flags: review?(mrosenberg) → review+
Attached patch Float32-full-support-part1.patch (obsolete) — Splinter Review
Attachment #8472974 - Attachment is obsolete: true
Attachment #8473560 - Flags: review?(mrosenberg)
Rebased patch so it can be applied.
Attachment #8468512 - Attachment is obsolete: true
Attachment #8473639 - Flags: review?(mrosenberg)
Attachment #8473639 - Flags: review?(mrosenberg) → review+
Attached patch Float32-full-support-part1.patch (obsolete) — Splinter Review
Rebased the patch after changes in some MIPS files.
Attachment #8473560 - Attachment is obsolete: true
Attachment #8473560 - Flags: review?(mrosenberg)
Attachment #8477305 - Flags: review?(mrosenberg)
Comment on attachment 8477305 [details] [diff] [review]
Float32-full-support-part1.patch

This patch already got r+ in comment #28. The changes made after this were to address the issues from comment #28 and rebase.
I'm carrying this r+.
Attachment #8477305 - Flags: review?(mrosenberg) → review+
Rebased patch: MOZ_ASSUME_UNREACHABLE -> MOZ_CRASH
Carry review from previous patch.
Attachment #8477305 - Attachment is obsolete: true
Attachment #8478979 - Flags: review+
Keywords: leave-open
Comment on attachment 8478979 [details] [diff] [review]
Float32-full-support-part1.patch

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

::: js/src/jit/mips/Architecture-mips.h
@@ +277,2 @@
>          MOZ_ASSERT(i < Total);
>          return GetName(Code(i));

If Total == 64, I think it should change to return GetName(Code(i % 32));
(In reply to Heiher from comment #35)
> ::: js/src/jit/mips/Architecture-mips.h
> @@ +277,2 @@
> >          MOZ_ASSERT(i < Total);
> >          return GetName(Code(i));
> 
> If Total == 64, I think it should change to return GetName(Code(i % 32));

Thanks for catching this. I will fix it in a new patch.
Attachment #8482735 - Flags: review?(mrosenberg)
Attachment #8482735 - Flags: review?(mrosenberg) → review+
https://hg.mozilla.org/mozilla-central/rev/1da7bca1c63d
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.