[PATCH] D27853: [CodeGenPrep]Restructure promoting Ext to form ExtLoad

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet added a comment.

Thanks for your patience.

I was on paternity leave and I am trying to catch up now.

Thanks for the clarification, I get the intend now.



================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:4273
     if (!TLI || !TLI->enableExtLdPromotion() || DisableExtLdPromotion)
-      continue;
+      return false;
+
----------------
junbuml wrote:
> qcolombet wrote:
> > Shouldn't we continue instead of returning?
> 
> The only reason we have this check inside the for loop is to catch the case where an extension is directly fed by a load because in such case the extension can be moved up without any promotion on its operands. For example, in the case below, we can move %s to BB0 even with DisableExtLdPromotion=true or TLI->enableExtLdPromotion()=false since no promotion is required:
>   BB0:
>     %ld = load 
>   BB1:
>     %s  = sext %ld
> I cannot see any case where the 2nd element in Exts is detected as a load after we do "continue" for the 1st element in Exts when the if statement (line 4272) is true. For example, in the case below, we cannot even promote %a if DisableExtLdPromotion=true :
>   BB0:
>     %ld = load 
>   BB1:
>     %a = add %ld, 1
>     %s  = sext %a
> 
Good point.
Add a comment about that.


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:4329
+          !(StressExtLdPromotion || NewCreatedInstsCost <= ExtCost ||
+           (ExtOperand->hasOneUse() || hasSameExtUse(ExtOperand, *TLI))))
+        continue;
----------------
junbuml wrote:
> qcolombet wrote:
> > I am confused we are checking that we can form an extended load and that is profitable.
> > The condition shouldn't use the ! operator, right?
> In this function (tryToPromoteExts()), we speculatively promote extensions and save extensions profitably moved up, in ProfitablyMovedExts.
> 
> Note that ProfitablyMovedExts contains profitable extensions which is not necessarily fed by a load. To find an extended load, we use canFormExtLd(), which will determine if the final output of ProfitablyMovedExts contains an extension which can be used to form an extendable load.
> 
> The final output of ProfitablyMovedExts after recursion will contain the last profitable extensions moved up during the speculative promotion, and it will be used in my second patch (https://reviews.llvm.org/D28680) which moves aarch64-type-promotion pass to CGP   
If we record all the extensions that are profitable shouldn't we do:

```
if (**!**isa<LoadInst>(ExtOperand) **||** !(StressExtLdPromotion || NewCreatedInstsCost <= ExtCost ||
           (ExtOperand->hasOneUse() || hasSameExtUse(ExtOperand, *TLI)))))
```

I.e., we should invert the first half of the original condition as well.


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:4336
+      TPT.rollback(LastKnownGood);
+      LastMovedExts.push_back(I);
       continue;
----------------
junbuml wrote:
> qcolombet wrote:
> > If NewPromoted is false, that means that everything is profitable, so we shouldn't rollback and or mark 'I' at the unprofitable point.
> > 
> > That part is somehow consistent with the inverted condition I mentioned, but given I cannot make sense out of that condition, I cannot make sense of that part either :P.
> If NewPromoted is false, that means none of speculative promotions for NewExts tried in the recursive call is profitable. So we should rollback and save the current extension (I) as its last profitable extension.
> 
The code makes sense now, I am still not convince about the initial condition though.
At the very least, that means you need to add comment to explain that part.


https://reviews.llvm.org/D27853