881390 - Hoist floating-point constants out of loops

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Dan Gohman [:sunfish]]

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.

Comment 1

[Dan Gohman [:sunfish]]

Attachment: a proposed fix

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.

Comment 2

[Hannes Verschore [:h4writer]]

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?

Comment 3

[Dan Gohman [:sunfish]]

My idea was that targets will likely want to diverge. ARM can load some floating-point constants in a single instruction, for example.

Comment 4

[Dan Gohman [:sunfish]]

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 5

[Hannes Verschore [:h4writer]]

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

Comment 6

[Hannes Verschore [:h4writer]]

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.

Comment 7

[Dan Gohman [:sunfish]]

Attachment: a new proposed fix

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

Comment 8

[Hannes Verschore [:h4writer]]

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)

Comment 9

[Dan Gohman [:sunfish]]

Attachment: updated for review comments

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

Comment 10

[Hannes Verschore [:h4writer]]

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

Comment 11

[Dan Gohman [:sunfish]]

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?

Comment 12

[Hannes Verschore [:h4writer]]

Yeah that's why I also suggested possiblyCalls as a MDefinition function ;)

Comment 13

[Dan Gohman [:sunfish]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/01824dc4f1ba

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/01824dc4f1ba