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

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet added inline comments.


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:4238
 
-/// \brief Try to form ExtLd by promoting \p Exts until they reach a
-/// load instruction.
-/// If an ext(load) can be formed, it is returned via \p LI for the load
-/// and \p Inst for the extension.
-/// Otherwise LI == nullptr and Inst == nullptr.
-/// When some promotion happened, \p TPT contains the proper state to
-/// revert them.
+/// \brief Try to promote \p extensions in /p Exts until they reach an
+/// unpromotable/unprofitable point. Save the last extensions which reach to an
----------------
\p ... /p => ... \p
Right?


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:4273
     if (!TLI || !TLI->enableExtLdPromotion() || DisableExtLdPromotion)
-      continue;
+      return false;
+
----------------
Shouldn't we continue instead of returning?


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


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


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


https://reviews.llvm.org/D27853