Closed Bug 1436691 Opened 2 years ago Closed 2 years ago

[MIPS32] Use ldc1 and sdc1 for double loads and stores

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

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

References

Details

Attachments

(1 file, 1 obsolete file)

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/51.0.2704.106 Safari/537.36
Attached patch bug1436691.patch (obsolete) — Splinter Review
This patch changes load/storeDouble to use an single ldc1/sdc1 instruction, performs minor cleanup and merges common peaces for mips64/mips32 code regarding double loads/stores.
Priority: -- → P5
Dragan, did you mean to request a review?
Flags: needinfo?(dragan.mladjenovic)
(In reply to Jason Orendorff [:jorendorff] from comment #2)
> Dragan, did you mean to request a review?

Sorry, I thought if I left it w/o review request somebody will pick it up. Feel free to delegate the review to whom you think is best.
Flags: needinfo?(dragan.mladjenovic)
Attachment #8949408 - Flags: review?(bbouvier)
Comment on attachment 8949408 [details] [diff] [review]
bug1436691.patch

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

Thanks, looks good!

Since you contribute often, and we can't thank you enough for this, may I suggest a few good practices for speeding up reviews. You will definitely help us review the code much faster if you can:
- group changes that are pure code motion into a separate patch (as in, the hoisting of methods from platform-specific to shared code)
- group changes that are only renaming things into a separate patch (eg the renaming of as_sd to as_sdc1)
- put other things that are independent in separate patches

Sometimes, it happens that we're refactoring code and see opportunities for other changes and include them in the current patch, and this can grow out of control, so it ends up being a big patch full of many changes that would have been better in separate patches. It's totally fine, it happens, sometimes! But it is certainly better if that's rather exceptional.
Thank you for your patches!

::: js/src/jit/mips-shared/MacroAssembler-mips-shared.cpp
@@ +1412,4 @@
>      adjustFrame(int32_t(sizeof(double)));
> +#else
> +    adjustFrame(f.size());
> +#endif

nit: shouldn't this be platform dependent, then? or better (see other comment in MIPS64 file), could all platforms not lie about the size they're actually pushing?

@@ +1430,4 @@
>      adjustFrame(-int32_t(sizeof(double)));
> +#else
> +    adjustFrame(-int32_t(f.size()));
> +#endif

ditto

::: js/src/jit/mips32/Trampoline-mips32.cpp
@@ +370,1 @@
>                     InvalidationBailoutStack::offsetOfFpRegs() + i * sizeof(double));

nit: spacing is wrong here (plus I'd add {} since the for loop body is on two lines)

@@ +583,1 @@
>                     BailoutStack::offsetOfFpRegs() + i * sizeof(double));

ditto

::: js/src/jit/mips64/MacroAssembler-mips64.cpp
@@ +852,5 @@
>          if (isLoongson()) {
>              as_gslsx(ft, address.base, ScratchRegister, 0);
>          } else {
>              as_daddu(ScratchRegister, address.base, ScratchRegister);
> +            as_lwc1(ft, ScratchRegister, 0);

Now the implementations of these functions could *almost* be shared, if not for the daddu vs addu difference...

@@ +914,2 @@
>  {
> +    as_ldc1(f, StackPointer, 0);

Can you specialize for double vs float32 as it's done on MIPS32 here? (and below for push)
It would remove the platform specific difference in MacroAssembler::Push/Pop that's in the shared code and that I've commented above.
Attachment #8949408 - Flags: review?(bbouvier) → review+
(In reply to Benjamin Bouvier [:bbouvier] from comment #4)
> Comment on attachment 8949408 [details] [diff] [review]
> bug1436691.patch
> 
> Review of attachment 8949408 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ....
> Sometimes, it happens that we're refactoring code and see opportunities for
> other changes and include them in the current patch, and this can grow out
> of control, so it ends up being a big patch full of many changes that would
> have been better in separate patches. It's totally fine, it happens,
> sometimes! But it is certainly better if that's rather exceptional.


Thanks, will do next time.

> ::: js/src/jit/mips-shared/MacroAssembler-mips-shared.cpp
> @@ +1412,4 @@
> >      adjustFrame(int32_t(sizeof(double)));
> > +#else
> > +    adjustFrame(f.size());
> > +#endif
> 
> nit: shouldn't this be platform dependent, then? or better (see other
> comment in MIPS64 file), could all platforms not lie about the size they're
> actually pushing?

The MIPS64 has to always push doubles in order to maintain the invariant that stack is always
pointer aligned. I could introduce something like FloatRegister::pushSize() to abstract this away?
 
> ::: js/src/jit/mips64/MacroAssembler-mips64.cpp
> @@ +852,5 @@
> >          if (isLoongson()) {
> >              as_gslsx(ft, address.base, ScratchRegister, 0);
> >          } else {
> >              as_daddu(ScratchRegister, address.base, ScratchRegister);
> > +            as_lwc1(ft, ScratchRegister, 0);
>

I would like to leave these two separate. Currently addu/daddu is only difference, but 
there are some addressing mode optimizations that only apply to mips32 but not to mips64
so these implementations might diverge more in the future.
Flags: needinfo?(bbouvier)
(In reply to Dragan Mladjenovic from comment #5)
> > ::: js/src/jit/mips-shared/MacroAssembler-mips-shared.cpp
> > @@ +1412,4 @@
> > >      adjustFrame(int32_t(sizeof(double)));
> > > +#else
> > > +    adjustFrame(f.size());
> > > +#endif
> > 
> > nit: shouldn't this be platform dependent, then? or better (see other
> > comment in MIPS64 file), could all platforms not lie about the size they're
> > actually pushing?
> 
> The MIPS64 has to always push doubles in order to maintain the invariant
> that stack is always
> pointer aligned. I could introduce something like FloatRegister::pushSize()
> to abstract this away?

Makes sense. Yes, either this or putting it down in the platform-specific code works for me, thanks.

>  
> > ::: js/src/jit/mips64/MacroAssembler-mips64.cpp
> > @@ +852,5 @@
> > >          if (isLoongson()) {
> > >              as_gslsx(ft, address.base, ScratchRegister, 0);
> > >          } else {
> > >              as_daddu(ScratchRegister, address.base, ScratchRegister);
> > > +            as_lwc1(ft, ScratchRegister, 0);
> >
> 
> I would like to leave these two separate. Currently addu/daddu is only
> difference, but 
> there are some addressing mode optimizations that only apply to mips32 but
> not to mips64
> so these implementations might diverge more in the future.

Makes sense too, and sounds cool.
Flags: needinfo?(bbouvier)
Assignee: nobody → dragan.mladjenovic
Attachment #8949408 - Attachment is obsolete: true
Attachment #8953400 - Flags: review+
Depends on: 1440626
Keywords: checkin-needed
Pushed by csabou@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d7874deef1a5
[MIPS32] Use ldc1 and sdc1 for double loads and stores; r=bbouvier
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d7874deef1a5
Status: UNCONFIRMED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.