[InstCombine] Fix inbounds gep for addrspacecasts

[svenvh]

Summary

The inbounds inference added by r277950 ("[InstCombine] Infer inbounds
on geps of allocas", 2016-08-07) could construct a new inbounds gep,
but ignore the address space.

Avoid asserting when the address space does not match. We could
introduce a new addrspacecast, but as one of the reviewers pointed
out, it is not clear that instcombine is allowed to introduce
addrspacecasts that weren't in the original program.

[loladiro]

Please see the discussion in D31924 - it's not clear that instcombine is allowed to introduce addrspacecasts that weren't in the original program. I know there's a couple cases like that in the code base already, but no need to add more.

[svenvh]

It seems commit r215467 ("Allwo bitcast + struct GEP transform to work with addrspacecast", 2014-08-12) already assumes it can swap GEPs and addrspacecasts indeed. Adding @arsenm; perhaps we should reconsider that commit?

In the meantime, I can update this patch to simply not infer inbounds when address spaces don't match, so that at least we no longer assert on the test case. Would that make sense?

[tauril]

Hello, I've recently come across the same problem as expressed by this revision.
Has there been any new discussions regarding this topic?

I've attached a small sample that triggers the type checking assertion in replaceAllUsesWith.

gep_not_inbounds.ll858 BDownload

[svenvh]

In D33361#1051795, @tauril wrote:

Hello, I've recently come across the same problem as expressed by this revision.
Has there been any new discussions regarding this topic?

Thanks for pinging this, Guillaume. I don't think we have reached a conclusion here, so I'll rework the patch to just avoid inferring inbounds for these cases.

[svenvh]

Updated patch to just bail out if address spaces don't match.

lib/Transforms/InstCombine/InstructionCombining.cpp

if (!isa<BitCastInst>(SrcOp) && GEP.accumulateConstantOffset(DL, Offset)) {
 
         if (NGEP->getType()->getPointerAddressSpace() != GEP.getAddressSpace())
           return new AddrSpaceCastInst(NGEP, GEPType);
         return new BitCastInst(NGEP, GEPType);
       }
     }
   }
 
+  // Try to infer inbounds on GEPs of allocas.
   if (!GEP.isInBounds()) {
     unsigned IdxWidth =
         DL.getIndexSizeInBits(PtrOp->getType()->getPointerAddressSpace());
     APInt BasePtrOffset(IdxWidth, 0);
     Value *UnderlyingPtrOp =
             PtrOp->stripAndAccumulateInBoundsConstantOffsets(DL,
                                                              BasePtrOffset);
     if (auto *AI = dyn_cast<AllocaInst>(UnderlyingPtrOp)) {
       if (GEP.accumulateConstantOffset(DL, BasePtrOffset) &&
           BasePtrOffset.isNonNegative()) {
         APInt AllocSize(IdxWidth, DL.getTypeAllocSize(AI->getAllocatedType()));
-        if (BasePtrOffset.ule(AllocSize)) {
+        if (BasePtrOffset.ule(AllocSize) && GEP.getAddressSpace() ==
+            PtrOp->getType()->getPointerAddressSpace()) {
           return GetElementPtrInst::CreateInBounds(
               PtrOp, makeArrayRef(Ops).slice(1), GEP.getName());
         }
       }
     }
   }
 
   return nullptr;

test/Transforms/InstCombine/getelementptr.ll

 ; CHECK-NEXT:    [[TMP1:%.*]] = getelementptr inbounds i32, i32* [[TMP0]], <2 x i64> <i64 0, i64 1>
 ; CHECK-NEXT:    ret <2 x i32*> [[TMP1]]
 ;
   %tmp0 = bitcast i32** %ptr to i32*
   %tmp1 = getelementptr inbounds i32, i32* %tmp0, <2 x i64> <i64 0, i64 1>
   ret <2 x i32*> %tmp1
 }
 
+; Test that inbounds inference does not crash in the presence of addrspacecasts.
+define i8 addrspace(42)* @gep_inbounds_addrspace() {
+; CHECK-LABEL: @gep_inbounds_addrspace
+; CHECK: addrspacecast
+; CHECK: getelementptr
+; CHECK: ret
+  %A = alloca <8 x half>, align 16
+  %B = bitcast <8 x half>* %A to i8*
+  %C = addrspacecast i8* %B to i8 addrspace(42)*
+  %gep = getelementptr i8, i8 addrspace(42)* %C, i64 12
+  ret i8 addrspace(42)* %gep
+}
+
 ; CHECK: attributes [[$NUW]] = { nounwind }