<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Thu, May 29, 2014 at 1:29 PM, Louis Gerbarg <span dir="ltr"><<a href="mailto:lgg@apple.com" target="_blank">lgg@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

Author: louis<br>
Date: Thu May 29 15:29:47 2014<br>
New Revision: 209843<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=209843&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=209843&view=rev</a><br>
Log:<br>
Add support for combining GEPs across PHI nodes<br>
<br>
Currently LLVM will generally merge GEPs. This allows backends to use more<br>
complex addressing modes. In some cases this is not happening because there<br>
is PHI inbetween the two GEPs:<br>
<br>
  GEP1--\<br>
        |-->PHI1-->GEP3<br>
  GEP2--/<br>
<br>
This patch checks to see if GEP1 and GEP2 are similiar enough that they can be<br>
cloned (GEP12) in GEP3's BB, allowing GEP->GEP merging (GEP123):<br>
<br>
  GEP1--\                     --\                           --\<br>
        |-->PHI1-->GEP3  ==>    |-->PHI2->GEP12->GEP3 == >    |-->PHI2->GEP123<br>
  GEP2--/                     --/                           --/<br>
<br>
This also breaks certain use chains that are preventing GEP->GEP merges that the<br>
the existing instcombine would merge otherwise.<br>
<br>
Tests included.<br>
<br>
Added:<br>
    llvm/trunk/test/Transforms/InstCombine/gepphigep.ll<br>
Modified:<br>
    llvm/trunk/lib/Transforms/InstCombine/InstructionCombining.cpp<br>
<br>
Modified: llvm/trunk/lib/Transforms/InstCombine/InstructionCombining.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/InstCombine/InstructionCombining.cpp?rev=209843&r1=209842&r2=209843&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/InstCombine/InstructionCombining.cpp?rev=209843&r1=209842&r2=209843&view=diff</a><br>


==============================================================================<br>
--- llvm/trunk/lib/Transforms/InstCombine/InstructionCombining.cpp (original)<br>
+++ llvm/trunk/lib/Transforms/InstCombine/InstructionCombining.cpp Thu May 29 15:29:47 2014<br>
@@ -1220,6 +1220,85 @@ Instruction *InstCombiner::visitGetEleme<br>
     if (MadeChange) return &GEP;<br>
   }<br>
<br>
+  // Check to see if the inputs to the PHI node are getelementptr instructions.</blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">+  if (PHINode *PN = dyn_cast<PHINode>(PtrOp)) {<br>

</blockquote><div><br></div><div>I think this comment can be made clearer, since the method is about GEPs, not about PHIs. So you could say that you're looking at GEPs whose pointer operands come from PHIs, which have other GEPs in their sources. Or even better, you could put your nice ASCII diagram in here, with the explanation. In a year or two when the code moves around enough, annotate/blame won't be helpful with providing the background here.</div>

<div><br></div><div>Also it would make sense to explicitly say what you mean by "similar enough" for two GEPs.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">


+    GetElementPtrInst *Op1 = dyn_cast<GetElementPtrInst>(PN->getOperand(0));<br>
+    if (!Op1)<br>
+      return nullptr;<br>
+<br>
+    signed DI = -1;<br></blockquote><div><br></div><div>Since DI is not a very descriptive name, perhaps a comment here would make the code clearer. What does DI hold and what's the significance of -1?</div><div> </div>

<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+<br>
+    for (auto I = PN->op_begin()+1, E = PN->op_end(); I !=E; ++I) {<br></blockquote><div><br></div><div>Spaces around !=</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">


+      GetElementPtrInst *Op2 = dyn_cast<GetElementPtrInst>(*I);<br>
+      if (!Op2 || Op1->getNumOperands() != Op2->getNumOperands())<br>
+        return nullptr;<br>
+<br></blockquote><div><br></div><div><snip></div><div><br></div><div>Eli</div></div></div></div>