[PATCH] D96399: [X86][CodeGenPrepare] Try to reuse IV's incremented value instead of adding the offset

[Philip Reames via Phabricator via llvm-commits]

reames requested changes to this revision.
reames added inline comments.
This revision now requires changes to proceed.


================
Comment at: llvm/lib/CodeGen/CodeGenPrepare.cpp:1279
 
+static bool isIVIncrement(const BinaryOperator *BO, const LoopInfo *LI) {
+  auto *PN = dyn_cast<PHINode>(BO->getOperand(0));
----------------
Can you precommit the extraction of the lambda to a static function?  The diff is confusing to read.


================
Comment at: llvm/lib/CodeGen/CodeGenPrepare.cpp:1299
                                                  Intrinsic::ID IID) {
-  auto isIVIncrement = [this, &Cmp](BinaryOperator *BO) {
-    auto *PN = dyn_cast<PHINode>(BO->getOperand(0));
-    if (!PN)
+  auto isSimpleIVIncrement = [this, &Cmp](BinaryOperator *BO) {
+    if (!isIVIncrement(BO, LI))
----------------
Same with the rename here.  (Or at least, this appears to be only a rename?)


================
Comment at: llvm/lib/CodeGen/CodeGenPrepare.cpp:3818
   ConstantInt *CI = nullptr; Value *AddLHS = nullptr;
-  if (isa<Instruction>(ScaleReg) &&  // not a constant expr.
+  if (isa<Instruction>(ScaleReg) && // not a constant expr.
       match(ScaleReg, m_Add(m_Value(AddLHS), m_ConstantInt(CI))) &&
----------------
Please remove whitespace change, feel free to commit separately.


================
Comment at: llvm/lib/CodeGen/CodeGenPrepare.cpp:3848
+    if (!IVInc || !L->contains(IVInc->getParent()))
+      return None;
+    ConstantInt *Step;
----------------
You really should be able to factor out some common code here with the isIVIncrement function above.  Maybe time for a getIVIncrement function?  

Hm, though there appears to be a bug in this copy.  The "incr" you identify doesn't appear to be tied to the PN.  


================
Comment at: llvm/lib/CodeGen/CodeGenPrepare.cpp:5103
     ExtAddrMode NewAddrMode = AddressingModeMatcher::Match(
-        V, AccessTy, AddrSpace, MemoryInst, AddrModeInsts, *TLI, *TRI,
-        InsertedInsts, PromotedInsts, TPT, LargeOffsetGEP, OptSize, PSI,
+        V, AccessTy, AddrSpace, MemoryInst, AddrModeInsts, *TLI, *LI, getDT(*F),
+        *TRI, InsertedInsts, PromotedInsts, TPT, LargeOffsetGEP, OptSize, PSI,
----------------
The need for domtree here introduces a potential compile time problem given the way CGP manages domtree invalidation.  Just noting it for now.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D96399/new/

https://reviews.llvm.org/D96399