[PATCH] D138637: [InstCombine] Combine opaque pointer single index GEP and with src GEP by matching the types

[krishna chaitanya sankisa via Phabricator via llvm-commits]

skc7 added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:2084
+  // GEP having single index.
+  if (PtrTy->isOpaquePointerTy() && Src->getResultElementType()->isArrayTy() &&
+      Src->getResultElementType() != GEP.getSourceElementType() &&
----------------
nikic wrote:
> The `Src->getResultElementType()->isArrayTy()` check here is overly strict. Consider that the result type is something like `{ [0 x float] }`. This can be handled the same way as `[0 x float]` with an extra zero index (which you do already handle, if this check is dropped).
> 
> > Src gep can have multiple users which can be geps. This patch tries to optimize them aswell. See gepofgep4 test added below.
> 
> Hm, it looks like the existing transform does allow GEPs with multiple uses here, but this seems like a bug to me, because this means that we will end up repeating non-trivial address calculations, rather than performing them only once.
Previously with non-opaque pointer case aswell, GEPs with multiple uses were optimized. Will there be any issue if we do so now with opaque pointers case ?
example with non opaque pointers and multiple uses case : https://llvm.godbolt.org/z/oEGE3736f 


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

https://reviews.llvm.org/D138637