[InstCombine] Remove a provenance changing gep transformation

[nagisa]

Summary

Ref: D93820: [InstSimplify] Don't fold gep p, -p to null and rG2ad1f5eb1a47: [InstCombine] Don't canonicalize (gep i8* X, -(ptrtoint Y)) as (inttoptr… are similar such changes landed recently.

https://alive2.llvm.org/ce/z/5KzdfL
https://alive2.llvm.org/ce/z/Sn-C-D

[nagisa]

I'm quite surprised this change didn't affect any pre-existing tests. It however, is sufficient to resolve one of a fairly easily hit mis-compiles in Rust.

(Side note: my understanding of this area of code is fairly limited, looking forward to learning something out of this)

[lebedev.ri]

Looks good to me.

llvm/lib/Transforms/InstCombine/InstructionCombining.cpp

2177–2178

This whole transform needs to go away, feel free to submit that as a preparatory patch.

[nagisa]

Rebase

[nagisa]

restore a deleted comment

[nikic]

The (in)correctness of this transform isn't really related to inbounds, see https://alive2.llvm.org/ce/z/Pr8daF. Actually, after @lebedev.ri's commit this code is now in sync with InstSimplify (both fold the ptrtoint-ptrtoint case, but not the 0-ptrtoint case).

The remaining fold (both here and in InstSimplify) is only valid if both pointers have the same provenance, i.e. getUnderlyingObject() is the same. I don't know what the fallout of enforcing that would be.

[nagisa]

The issue this was trying to solve was effectively resolved by rG2ad1f5eb1a47: [InstCombine] Don't canonicalize (gep i8* X, -(ptrtoint Y)) as (inttoptr…. I'm happy to abandon this, because the change to the other transformation is of questionable correctness anyway.

Thanks @lebedev.ri!

[nagisa]

Pivot to removing the provenance losing transformation

[nagisa]

lint

[lebedev.ri]

In D98120#2609269, @nikic wrote:

The remaining fold (both here and in InstSimplify) is only valid if both pointers have the same provenance, i.e. getUnderlyingObject() is the same.

That's a good point, this fold should be somewhat good iff we can prove that it doesn't miscompile.

In D98120#2609269, @nikic wrote:

I don't know what the fallout of enforcing that would be.

Ack.

llvm/lib/Transforms/InstCombine/InstructionCombining.cpp

if (!Indices.empty())
                        GEP.getName())
                  : GetElementPtrInst::Create(Src->getSourceElementType(),
                                              Src->getOperand(0), Indices,
                                              GEP.getName());
   }
 
   // Skip if GEP source element type is scalable. The type alloc size is unknown
   // at compile-time.
-  if (GEP.getNumIndices() == 1 && !IsGEPSrcEleScalable) {
+  // Also skip for non-inbounds GEPs, because the optimization inside is not
+  // correct for regular GEPs.
+  if (GEP.getNumIndices() == 1 && !IsGEPSrcEleScalable && GEP.isInBounds()) {
     unsigned AS = GEP.getPointerAddressSpace();
     if (GEP.getOperand(1)->getType()->getScalarSizeInBits() ==
         DL.getIndexSizeInBits(AS)) {
       uint64_t TyAllocSize = DL.getTypeAllocSize(GEPEltType).getFixedSize();
 
       bool Matched = false;
       uint64_t C;
       Value *V = nullptr;
       if (TyAllocSize == 1) {
         V = GEP.getOperand(1);
         Matched = true;
       } else if (match(GEP.getOperand(1),
                        m_AShr(m_Value(V), m_ConstantInt(C)))) {
         if (TyAllocSize == 1ULL << C)
           Matched = true;
       } else if (match(GEP.getOperand(1),
                        m_SDiv(m_Value(V), m_ConstantInt(C)))) {
         if (TyAllocSize == C)
           Matched = true;
       }
 
       if (Matched) {
-        // Canonicalize (gep i8* X, (ptrtoint Y)-(ptrtoint X))
         // to (bitcast Y)
         Value *Y;
         if (match(V, m_Sub(m_PtrToInt(m_Value(Y)),
                            m_PtrToInt(m_Specific(GEP.getOperand(0))))))
           return CastInst::CreatePointerBitCastOrAddrSpaceCast(Y, GEPType);
       }
     }
   }
 

llvm/test/Other/regr-gep-provenance.ll

+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt < %s -S -O3 -o - | FileCheck %s
+
+; This test verifies that we do not optimize GEPs in a way that would lose pointer provenance (e.g.
+; by optimizing a series of operations that result in `null` into a `null` (which does not have
+; any provenance in particular)
+
+define i8 @apple(i8* %x) unnamed_addr #0 {
+; CHECK-LABEL: @apple(
+; CHECK-NEXT:    [[E:%.*]] = load i8, i8* [[X:%.*]], align 1
+; CHECK-NEXT:    ret i8 [[E]]
+;
+  %a = ptrtoint i8* %x to i64
+  %b = sub i64 0, %a
+  %c = getelementptr i8, i8* %x, i64 %b
+  %d = getelementptr i8, i8* %c, i64 %a
+  %e = load i8, i8* %d, align 1
+  ret i8 %e
+}
+
+define i8* @banana(i8* %b) {
+; CHECK-LABEL: @banana(
+; CHECK-NEXT:    [[B_PTR:%.*]] = ptrtoint i8* [[B:%.*]] to i64
+; CHECK-NEXT:    [[SUB:%.*]] = sub i64 0, [[B_PTR]]
+; CHECK-NEXT:    [[GEP:%.*]] = getelementptr i8, i8* [[B]], i64 [[SUB]]
+; CHECK-NEXT:    ret i8* [[GEP]]
+;
+  %b_ptr = ptrtoint i8* %b to i64
+  %sub = sub i64 0, %b_ptr
+  %gep = getelementptr i8, i8* %b, i64 %sub
+  ret i8* %gep
+}