Closed Bug 1213747 Opened 5 years ago Closed 5 years ago

IonMonkey: MIPS64: Import MoveEmitter-mips64

Categories

(Core :: JavaScript Engine: JIT, defect)

Other
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed
b2g-v2.5 --- fixed

People

(Reporter: hev, Assigned: hev)

References

Details

Attachments

(2 files, 1 obsolete file)

Bug 1140954, Part 9: MoveEmitter-mips64.
Thanks!
Attachment #8672508 - Flags: review?(nicolas.b.pierron)
Attachment #8672508 - Flags: review?(branislav.rankov)
Thanks!
Attachment #8672509 - Flags: review?(nicolas.b.pierron)
Attachment #8672509 - Flags: review?(branislav.rankov)
Comment on attachment 8672508 [details] [diff] [review]
Part 9.1: Import MIPS64 support into MoveEmitter-mips-shared.

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

::: js/src/jit/mips-shared/MoveEmitter-mips-shared.h
@@ +36,5 @@
>  
>      void assertDone();
>      Register tempReg();
>      FloatRegister tempFloatReg();
> +    Address cycleSlot(uint32_t slot, uint32_t subslot = 0) const;

This modification does not seems to be used in this patch, undo this modification and move it to the proper patch.
Attachment #8672508 - Flags: review?(nicolas.b.pierron) → review+
Comment on attachment 8672509 [details] [diff] [review]
Part 9.2: Import MoveEmitter-mips64.

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

::: js/src/jit/mips64/MoveEmitter-mips64.cpp
@@ +28,5 @@
> +            masm.loadFloat32(getAdjustedAddress(to), temp);
> +            masm.storeFloat32(temp, cycleSlot(slotId));
> +        } else {
> +            // Just always store the largest possible size.
> +            masm.storeDouble(to.floatReg(), cycleSlot(slotId));

Can you clarify this one to me?

@@ +152,5 @@
> +            masm.loadPtr(getAdjustedAddress(from), to.reg());
> +        } else {
> +            // Used for moving a double parameter from the same source. See Bug 1123874.
> +            if(to.reg() == a2 || to.reg() == a3)
> +                masm.ma_move(to.reg(), from.reg());

Note, in a follow-up patch, you should look on how to use REG_PAIR to express this in terms of a move operand.
(In reply to Nicolas B. Pierron [:nbp] from comment #4)
> Comment on attachment 8672509 [details] [diff] [review]
> Part 9.2: Import MoveEmitter-mips64.
> 
> Review of attachment 8672509 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit/mips64/MoveEmitter-mips64.cpp
> @@ +28,5 @@
> > +            masm.loadFloat32(getAdjustedAddress(to), temp);
> > +            masm.storeFloat32(temp, cycleSlot(slotId));
> > +        } else {
> > +            // Just always store the largest possible size.
> > +            masm.storeDouble(to.floatReg(), cycleSlot(slotId));
> 
> Can you clarify this one to me?
Looks is a mistake, I think just need store float32 on mips64.

> 
> @@ +152,5 @@
> > +            masm.loadPtr(getAdjustedAddress(from), to.reg());
> > +        } else {
> > +            // Used for moving a double parameter from the same source. See Bug 1123874.
> > +            if(to.reg() == a2 || to.reg() == a3)
> > +                masm.ma_move(to.reg(), from.reg());
> 
> Note, in a follow-up patch, you should look on how to use REG_PAIR to
> express this in terms of a move operand.
Oh, This code doesn't needed on mips64, because there have eight float registers for arguments passing, and general arguments registers count is eight, too. I think move data from float register to general register is unnecessary.
Clean up unnecessary code.
Attachment #8672509 - Attachment is obsolete: true
Attachment #8672509 - Flags: review?(nicolas.b.pierron)
Attachment #8672509 - Flags: review?(branislav.rankov)
Attachment #8676046 - Flags: review?(branislav.rankov)
Attachment #8676046 - Flags: review?(nicolas.b.pierron)
Attachment #8676046 - Flags: review?(arai.unmht)
Comment on attachment 8676046 [details] [diff] [review]
Part 9.2: Import MoveEmitter-mips64.

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

I'm not capable of reviewing this, sorry.
Attachment #8676046 - Flags: review?(arai.unmht)
(In reply to Tooru Fujisawa [:arai] from comment #10)
> Comment on attachment 8676046 [details] [diff] [review]
> Part 9.2: Import MoveEmitter-mips64.
> 
> Review of attachment 8676046 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm not capable of reviewing this, sorry.

It doesn't matter, thank you! ;)
Attachment #8676046 - Flags: review?(nicolas.b.pierron) → review+
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.