[llvm] r209843 - Add support for combining GEPs across PHI nodes

[Eli Bendersky]

On Thu, May 29, 2014 at 1:29 PM, Louis Gerbarg <lgg at apple.com> wrote:

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

+  if (PHINode *PN = dyn_cast<PHINode>(PtrOp)) {
>

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.

Also it would make sense to explicitly say what you mean by "similar
enough" for two GEPs.


> +    GetElementPtrInst *Op1 =
> dyn_cast<GetElementPtrInst>(PN->getOperand(0));
> +    if (!Op1)
> +      return nullptr;
> +
> +    signed DI = -1;
>

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?


> +
> +    for (auto I = PN->op_begin()+1, E = PN->op_end(); I !=E; ++I) {
>

Spaces around !=


> +      GetElementPtrInst *Op2 = dyn_cast<GetElementPtrInst>(*I);
> +      if (!Op2 || Op1->getNumOperands() != Op2->getNumOperands())
> +        return nullptr;
> +
>

<snip>

Eli
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140530/2ef3b2c3/attachment.html>