<div dir="ltr">FWIW: In gcc, the regeneration is a pair of mutually recursive functions, one called "create_expression_by_pieces" (which is the main interface), and one called "find_or_generate_expression"  (which in turn, calls create_expression_by_pieces for any operand that is itself a complex value expression).<div><br></div><div>Whether you do it similarly, or worklist it, you should have some invariant that each iteration guarantees or whatever.</div><div><br></div><div>I found mutual recursion easier because you can easily guarantee "at the end of this function we return the thing i need to place in this expression" for one of the functions, and there you go.</div><div><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Jul 20, 2016 at 3:10 PM, David Majnemer <span dir="ltr"><<a href="mailto:david.majnemer@gmail.com" target="_blank">david.majnemer@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">majnemer added inline comments.<br>
<br>
================<br>
Comment at: llvm/lib/Transforms/Scalar/GVNHoist.cpp:578-603<br>
@@ +577,28 @@<br>
+  // Make all operands of the GEP available.<br>
+  bool makeGepOperandsAvailable(GetElementPtrInst *Gep, BasicBlock *HoistPt,<br>
+                                Instruction *Repl) const {<br>
+    for (unsigned i = 0, e = Gep->getNumOperands(); i != e; ++i)<br>
+      if (Instruction *Op = dyn_cast<Instruction>(Gep->getOperand(i))) {<br>
+<br>
+        // Check whether the operand is already available.<br>
+        if (DT->dominates(Op->getParent(), HoistPt))<br>
+          continue;<br>
+<br>
+        // As a GEP can refer to other GEPs, recursively check whether all the<br>
+        // operands of this GEP can be computed at HoistPt.<br>
+        if (GetElementPtrInst *GepOp = dyn_cast<GetElementPtrInst>(Op))<br>
+          if (!makeGepOperandsAvailable(GepOp, HoistPt, Gep))<br>
+            return false;<br>
+<br>
+        // Failed to make Op available.<br>
+        return false;<br>
+      }<br>
+<br>
+    // Copy Gep and replace its uses in Repl with ClonedGep.<br>
+    Instruction *ClonedGep = Gep->clone();<br>
+    ClonedGep->insertBefore(HoistPt->getTerminator());<br>
+    Repl->replaceUsesOfWith(Gep, ClonedGep);<br>
+<br>
+    return true;<br>
+  }<br>
+<br>
----------------<br>
IIUC, this can mutate the IR while still returning false.<br>
<br>
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).<br>
<div class="HOEnZb"><div class="h5"><br>
<br>
Repository:<br>
  rL LLVM<br>
<br>
<a href="https://reviews.llvm.org/D22599" rel="noreferrer" target="_blank">https://reviews.llvm.org/D22599</a><br>
<br>
<br>
<br>
</div></div></blockquote></div><br></div>