[PATCH] [MachineLICM] Sink instructions only if they are unlikely to be executed

[Quentin Colombet]

Hi Daniel,

> Register spills have other costs as well. At the very least, they are increasing the stack frame size and incur the cost of writing to the stack. If the instructions are only executed a handful of times, as in the case when they are unlikely to be executed, this additional cost is significant.


Ok I see where you are going now. That being said, I think that the overhead of stores + stack update (i.e., what we are talking about) is not that relevant. Anyway, I agree that it should be taken into account at some point.

> You are saying that we should sink all the instructions where C is negative (spill reload is cheaper).


The opposite :). We should *not* sink those instructions. Of course, we agree that this is useless to sink if register pressure is not a problem, which we do not check at all here.

>   Now, for positive C, we should sink instructions if C*L is smaller than some threshold that factors in the cost of the spill itself which I mentioned above. It is the latter part that I am getting at with this patch and I do think that looking at probabilities is a step into the right direction (although maybe not the most important first step).


Aside from register pressure, which now I understand you want to consider later, I am still not convinced that the suggested check represent a meaningful cost model.
Anyhow, we can rework that when we will look at the register pressure thing.
The bottom line is I am fine with whatever path you think is worth pursuing.

Cheers,
-Quentin


================
Comment at: lib/CodeGen/MachineLICM.cpp:835
@@ -818,8 +834,3 @@
       }
-      if (!B) {
-        B = MI.getParent();
-        continue;
-      }
-      B = DT->findNearestCommonDominator(B, MI.getParent());
-      if (!B) {
-        CanSink = false;
+      B = B ? DT->findNearestCommonDominator(B, MI.getParent())
+            : MI.getParent();
----------------
djasper wrote:
> qcolombet wrote:
> > Aborting on PHI does not make much sense.
> > Instead, you should look for the common dominator for the use of the PHI, e.g., by looking at the dominator of the terminator of the related block. In that case, you wouldn’t insert at after the first non-PHI instruction.
> I don't think I understand this.
> 
> We are talking about an instruction that is currently in the loop preheader and its use is in a PHI. Doesn't that (at least in the vast majority of cases) mean that we are needing the value only when branching from the loop preheader into the loop? Thus, the instruction is currently at the best possible place.
Shouldn’t have we filter out those cases with this check:
!HasLoopPHIUse(I)

I thought we were speaking of a PHI within a diamond in the loop, not the PHI of the header.

http://reviews.llvm.org/D8451

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/