[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:2040
+        (Src->hasAllConstantIndices() && !GEP.hasAllConstantIndices())) {
+      bool InBounds = isMergedGEPInBounds(*Src, *cast<GEPOperator>(&GEP));
+      Value *NewSrc = Builder.CreateGEP(
----------------
huangjd wrote:
> nikic wrote:
> > Unfortunately, this is not sufficient to preserve inbounds. If the indices have different sign, then swapping them may make the GEP non-inbounds even if both original GEPs were inbounds.
> > 
> > `gep inbounds (gep inbounds p, -1), X -> `gep inbounds (gep inbounds p, X), -1` is not generally valid.
> I have seen the same inbounds check used in another place (line 2071), so that may be problematic as well? I think we could add another check in isMergedGEPInBounds to require both GEP offsets are known to be same sign? 
Is the impact of changing inbounds GEP to non-inbounds significant to other optimizations? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125845