Closed Bug 881390 Opened 6 years ago Closed 6 years ago

Hoist floating-point constants out of loops

Categories

(Core :: JavaScript Engine, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: sunfish, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

For example in the testcase in bug 876057, it can be useful to let LICM hoist floating-point constants out of loops.

Hoisting can increase register pressure, however in many cases spilling the floating-point constant and reloading it inside the loop will still be cheaper than rematerializing the value inside the loop.
Attached patch a proposed fix (obsolete) — Splinter Review
The value of this optimization is obviously very target-specific. To account for this, this patch introduces a new framework for allowing target-specific hooks on the MIRGenerator class.

I'm open to suggestions of other ways of approaching this, of course.
Attachment #760524 - Flags: review?(hv1989)
Blocks: 876057
Comment on attachment 760524 [details] [diff] [review]
a proposed fix

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

Haven't reviewed yet, but I don't understand why we have this MIRGenerator specific stuff, when the function isExpensiveConstant is the same for all platforms?
My idea was that targets will likely want to diverge. ARM can load some floating-point constants in a single instruction, for example.
I discussed this with h4writer on irc, and he suggested we fix bug 876064 first, and possibly wait on hoisting until the register allocator is capable of rematerializing hoisted values, to avoid trouble with register pressure.
Comment on attachment 760524 [details] [diff] [review]
a proposed fix

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

Clearing review for now until bug 876064 lands
Attachment #760524 - Flags: review?(hv1989)
Since bug 876064 landed, I think this is more pressing now?! I still think we should try to keep the MIR platform independent. So for now I think it would make sense to add a virtual function to MDefinition, "onlyHoistIfUseIsHoistable" (feel free to find better name ;)) and return false for everything except for MConstantElements, MConstant (non-float) and MBox.
Attached patch a new proposed fix (obsolete) — Splinter Review
If it's not target-dependent, then it seems simpler for the predicate to just live in LICM.cpp for now. This patch does that, and adds a simple heuristic to avoid hoisting floating-point constants out of loops containing calls, since calls often significantly increase the register pressure of the loop (effectively).
Attachment #760524 - Attachment is obsolete: true
Attachment #777865 - Flags: review?(hv1989)
Comment on attachment 777865 [details] [diff] [review]
a new proposed fix

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

::: js/src/ion/LICM.cpp
@@ +182,5 @@
>  
> +            // Remember whether this loop contains anything which clobbers most
> +            // or all floating-point registers. This is just a rough heuristic.
> +            if (ins->isCall() || ins->isAsmJSCall())
> +                containsFPClobber_ = true;

This isn't correct. We have much more "Call" instrunction. Look at all instruction that use LCallInstructionHelper. I think the best way to solve this is to add a virtual function to MIR "isCallInstruction" and default it to false, but overide it for every instruction that has a LIR with LCallInstruction and flag it true.

@@ +249,5 @@
> +        // Do hoist floating-point constants, but only if the loop doesn't
> +        // contain anything likely to cause them to spill.
> +        if (ins->isConstantElements() || ins->isBox() ||
> +            (ins->isConstant() && (ins->type() != MIRType_Double || containsFPClobber_)))
> +        {

Since this is getting bigger and bigger, lets create a function for it:
Loop::requiresHoistedUse(MDefinition *ins)
Attachment #777865 - Flags: review?(hv1989)
(In reply to Hannes Verschore [:h4writer] from comment #8)
> Comment on attachment 777865 [details] [diff] [review]
> a new proposed fix
> 
> Review of attachment 777865 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/ion/LICM.cpp
> @@ +182,5 @@
> >  
> > +            // Remember whether this loop contains anything which clobbers most
> > +            // or all floating-point registers. This is just a rough heuristic.
> > +            if (ins->isCall() || ins->isAsmJSCall())
> > +                containsFPClobber_ = true;
> 
> This isn't correct. We have much more "Call" instrunction. Look at all
> instruction that use LCallInstructionHelper. I think the best way to solve
> this is to add a virtual function to MIR "isCallInstruction" and default it
> to false, but overide it for every instruction that has a LIR with
> LCallInstruction and flag it true.

I updated the patch to implement this. It still isn't perfect though, because some nodes are conditionally lowered to calls, and I didn't think it was worth duplicating the conditions or factoring them out for this.

> @@ +249,5 @@
> > +        // Do hoist floating-point constants, but only if the loop doesn't
> > +        // contain anything likely to cause them to spill.
> > +        if (ins->isConstantElements() || ins->isBox() ||
> > +            (ins->isConstant() && (ins->type() != MIRType_Double || containsFPClobber_)))
> > +        {
> 
> Since this is getting bigger and bigger, lets create a function for it:
> Loop::requiresHoistedUse(MDefinition *ins)

Ok.
Attachment #777865 - Attachment is obsolete: true
Attachment #779244 - Flags: review?(hv1989)
Comment on attachment 779244 [details] [diff] [review]
updated for review comments

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

Awesome, this looks good! I agree that the function "isFPClobber" on the MIR is more of hint. So might be wrong (maybe add this to the virtual function description.
r+ with the requested name change.

::: js/src/ion/MIR.h
@@ +343,5 @@
>  
> +    // Also for LICM. Test whether this definition is likely to clobber all or
> +    // many of the floating-point registers, such that hoisting floating-point
> +    // constants out of containing loops isn't likely to be worthwhile.
> +    virtual bool isFPClobber() const { return false; }

This is very specific name and don't think we will use it somewhere else.
That's why I suggested to use isCall / possiblyCalls here. That might get used for other things...
Attachment #779244 - Flags: review?(hv1989) → review+
isCall doesn't work because there's already a member function by that name (for MCall). Do you suggest possiblyCalls for the MDefinition member function or something else? Otherwise, do you have a different suggestion for a name for that member function?
Yeah that's why I also suggested possiblyCalls as a MDefinition function ;)
https://hg.mozilla.org/mozilla-central/rev/01824dc4f1ba
Status: UNCONFIRMED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.