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

[Jun Bum Lim via Phabricator via llvm-commits]

junbuml added inline comments.


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:4273
     if (!TLI || !TLI->enableExtLdPromotion() || DisableExtLdPromotion)
-      continue;
+      return false;
+
----------------
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



================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:4329
+          !(StressExtLdPromotion || NewCreatedInstsCost <= ExtCost ||
+           (ExtOperand->hasOneUse() || hasSameExtUse(ExtOperand, *TLI))))
+        continue;
----------------
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   


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:4331
+        continue;
+      LastMovedExts.push_back(MovedExt);
+      NewPromoted = true;
----------------
qcolombet wrote:
> I get that this MovedExt is not profitable only if the previous condition is inverted (the ! operator).
At this point MovedExt is profitable, so save it.


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:4336
+      TPT.rollback(LastKnownGood);
+      LastMovedExts.push_back(I);
       continue;
----------------
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.



https://reviews.llvm.org/D27853