[PATCH] D22599: GVN-hoist: improve code generation for recursive GEPs

[Sebastian Pop via llvm-commits]

sebpop added inline comments.

================
Comment at: llvm/lib/Transforms/Scalar/GVNHoist.cpp:578-603
@@ +577,28 @@
+  // Make all operands of the GEP available.
+  bool makeGepOperandsAvailable(GetElementPtrInst *Gep, BasicBlock *HoistPt,
+                                Instruction *Repl) const {
+    for (unsigned i = 0, e = Gep->getNumOperands(); i != e; ++i)
+      if (Instruction *Op = dyn_cast<Instruction>(Gep->getOperand(i))) {
+
+        // Check whether the operand is already available.
+        if (DT->dominates(Op->getParent(), HoistPt))
+          continue;
+
+        // As a GEP can refer to other GEPs, recursively check whether all the
+        // operands of this GEP can be computed at HoistPt.
+        if (GetElementPtrInst *GepOp = dyn_cast<GetElementPtrInst>(Op))
+          if (!makeGepOperandsAvailable(GepOp, HoistPt, Gep))
+            return false;
+
+        // Failed to make Op available.
+        return false;
+      }
+
+    // Copy Gep and replace its uses in Repl with ClonedGep.
+    Instruction *ClonedGep = Gep->clone();
+    ClonedGep->insertBefore(HoistPt->getTerminator());
+    Repl->replaceUsesOfWith(Gep, ClonedGep);
+
+    return true;
+  }
+
----------------
majnemer wrote:
> IIUC, this can mutate the IR while still returning false.
> 
> I'd change this to be a worklist algorithm instead of a recursive one.  Inside the worklist loop, I'd push GEP candidates onto a vector for later processing (cloning).
You may be right: I'm not sure whether GEP operands are allowed to contain more than one GEP, in which case we may change the IR for one operand and then finally return false because of another GEP for which we fail to make operands available.

As the suggested change is a good cleanup, I will update the patch.

Thanks for your review.


Repository:
  rL LLVM

https://reviews.llvm.org/D22599