[PATCH] D110246: [InstSimplify][InstCombine] Fold ptrtoint(gep i8 null, x) -> x

[Alexander Richardson via Phabricator via llvm-commits]

arichardson added inline comments.


================
Comment at: llvm/lib/Analysis/InstructionSimplify.cpp:4682
+        // limited to cases where the result type matches the GEP index type.
+        if (GEP->getNumIndices() == 1 && GEP->getOperand(1)->getType() == Ty) {
+          // We also can't perform the fold if we need a type size multiplier.
----------------
lebedev.ri wrote:
> https://godbolt.org/z/hTbv3qqGT
Sorry, I don't quite understand what this link is showing. The base is not null and there is more than one GEP index so this transformation won't happen.


================
Comment at: llvm/lib/Analysis/InstructionSimplify.cpp:4685
+        if (GEP->getNumIndices() == 1 && GEP->getOperand(1)->getType() == Ty &&
+            Q.DL.getTypeAllocSize(GEP->getSourceElementType()) == 8)
+          return GEP->getOperand(1);
----------------
lebedev.ri wrote:
> arichardson wrote:
> > lebedev.ri wrote:
> > > This is surely broken
> > In what way? It's here to ensure that the following isn't incorrectly folded to %val:
> > ```
> > ; We can't fold non-i8 GEPs in InstSimplify since that would require adding new arithmetic.
> > ; TODO: handle this case in InstCombine
> > define i64 @fold_ptrtoint_nullgep_i32_variable(i64 %val) {
> >   %ptr = getelementptr i32, i32 addrspace(1)* null, i64 %val
> >   %ret = ptrtoint i32 addrspace(1)* %ptr to i64
> >   ret i64 %ret
> > }
> > ```
> > 
> Why 8 and not 42?
Good catch, that was a bad copy-paste error from TypeSizeInBits().

If I understand the GEP logic correctly, then TypeAllocSize == 1 implies there is no multiplier and we can therefore return the index unchanged.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110246