[PATCH] D125845: [InstCombine] Canonicalize GEP of GEP by swapping constant-indexed GEP to the back

[William Junda Huang via Phabricator via llvm-commits]

huangjd added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:1960-1964
+  // LICM takes priority over canonicalization swapping for merging constant-
+  // indexed GEP, because mergable constant-indexed GEPs can still be merged
+  // after they are hoisted out of the loop, but performing canonicalization
+  // first may miss the LICM opportunity.
+  bool shouldCanonicalizeSwap = true;
----------------
spatel wrote:
> This code structure and the comments are confusing. I think it would be better to rearrange this like:
>   bool LoopInvariantGEP = false;
>   bool LoopInvariantSrc = false;
>   if (LI && ...) {
>     LoopInvariantGEP = L->isLoopInvariant(GEP.getOperand(1));
>     LoopInvariantSrc = L->isLoopInvariant(Src->getOperand(1));
>   }
>   if (LoopInvariantGEP && !LoopInvariantSrc) {
>     // do the existing transform
>   }
>   if (!(!LoopInvariantGEP && LoopInvariantSrc) && ...) {
>     // do the new transform
>   }
> 
> I think that matches the logic in this patch, but don't we need a set of tests with loops to make sure that does what you expect? I don't see any tests with loops.
I think this doesn't simplify the logic, because LoopInvariant* is only meaningful within the scope of LoopInfo analysis. Also the existing transform (LICM) requires the Loop object from LI, so moving it outside of the first condition statement makes the code uglier. 


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:1960-1964
+  // LICM takes priority over canonicalization swapping for merging constant-
+  // indexed GEP, because mergable constant-indexed GEPs can still be merged
+  // after they are hoisted out of the loop, but performing canonicalization
+  // first may miss the LICM opportunity.
+  bool shouldCanonicalizeSwap = true;
----------------
huangjd wrote:
> spatel wrote:
> > This code structure and the comments are confusing. I think it would be better to rearrange this like:
> >   bool LoopInvariantGEP = false;
> >   bool LoopInvariantSrc = false;
> >   if (LI && ...) {
> >     LoopInvariantGEP = L->isLoopInvariant(GEP.getOperand(1));
> >     LoopInvariantSrc = L->isLoopInvariant(Src->getOperand(1));
> >   }
> >   if (LoopInvariantGEP && !LoopInvariantSrc) {
> >     // do the existing transform
> >   }
> >   if (!(!LoopInvariantGEP && LoopInvariantSrc) && ...) {
> >     // do the new transform
> >   }
> > 
> > I think that matches the logic in this patch, but don't we need a set of tests with loops to make sure that does what you expect? I don't see any tests with loops.
> I think this doesn't simplify the logic, because LoopInvariant* is only meaningful within the scope of LoopInfo analysis. Also the existing transform (LICM) requires the Loop object from LI, so moving it outside of the first condition statement makes the code uglier. 
It seems that LI is null when I test it. Is there an opt flag I need to enable? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125845