InstCombine: Fix crash on icmp of gep with addrspacecasted null

[jdoerfert]

lib/Transforms/InstCombine/InstCombineCompares.cpp

908

Why doesn't the NullPointerIsDefined check catch this and prevent the transformation in the first place?

[arsenm]

lib/Transforms/InstCombine/InstCombineCompares.cpp

908

Because this is valid. This stripped off the addrspacecast and sees this is a cast from the invalid null in addrspace 0.

[jdoerfert]

test/Transforms/InstCombine/gep-inbounds-null.ll

223

I somehow feel we should transform this into ret i1 false instead of something we cannot lower into a constant anymore.

However, I see the problem and the fix seems fine. Could you add the following test as well and a fixme above to denote this should be ret i1 false?

define i1 @invalid_bitcast_icmp_addrspacecast_as0_null_var(i32 addrspace(5)* %ptr, i32 %idx) {
; CHECK-LABEL: @invalid_bitcast_icmp_addrspacecast_as0_null(
; CHECK-NEXT:  bb:
; CHECK-NEXT:    [[TMP2:%.*]] = icmp eq i32 addrspace(5)* [[PTR:%.*]], addrspacecast (i32* null to i32 addrspace(5)*)
; CHECK-NEXT:    ret i1 [[TMP2]]
;
bb:
  %tmp1 = getelementptr inbounds i32, i32 addrspace(5)* %ptr, i32 %idx
  %tmp2 = icmp eq i32 addrspace(5)* %tmp1, addrspacecast (i32* null to i32 addrspace(5)*)
  ret i1 %tmp2
}

[arsenm]

test/Transforms/InstCombine/gep-inbounds-null.ll

223

Actually, this did fold to false in my original test. I might have broken something when I copied it into the preexisting test

[arsenm]

Add test and fixme

test/Transforms/InstCombine/gep-inbounds-null.ll

223

My original test was comparing to an alloca derived pointer, so I believe it was deleted for other reasons

[jdoerfert]

Thx. LGTM.

[arsenm]

r371146

lib/Transforms/InstCombine/InstCombineCompares.cpp

if (!Offset)
       Offset = EmitGEPOffset(GEPLHS);
     return new ICmpInst(ICmpInst::getSignedPredicate(Cond), Offset,
                         Constant::getNullValue(Offset->getType()));
   }
 
   if (GEPLHS->isInBounds() && ICmpInst::isEquality(Cond) &&
       isa<Constant>(RHS) && cast<Constant>(RHS)->isNullValue() &&
       !NullPointerIsDefined(I.getFunction(),
                             RHS->getType()->getPointerAddressSpace())) {
     // For most address spaces, an allocation can't be placed at null, but null
     // itself is treated as a 0 size allocation in the in bounds rules.  Thus,
     // the only valid inbounds address derived from null, is null itself.
     // Thus, we have four cases to consider:
     // 1) Base == nullptr, Offset == 0 -> inbounds, null
     // 2) Base == nullptr, Offset != 0 -> poison as the result is out of bounds
     // 3) Base != nullptr, Offset == (-base) -> poison (crossing allocations)
     // 4) Base != nullptr, Offset != (-base) -> nonnull (and possibly poison)
     //
     // (Note if we're indexing a type of size 0, that simply collapses into one
     //  of the buckets above.)
     //
     // In general, we're allowed to make values less poison (i.e. remove
     //   sources of full UB), so in this case, we just select between the two
     //   non-poison cases (1 and 4 above).
     //
     // For vectors, we apply the same reasoning on a per-lane basis.
     auto *Base = GEPLHS->getPointerOperand();
     if (GEPLHS->getType()->isVectorTy() && Base->getType()->isPointerTy()) {
       int NumElts = GEPLHS->getType()->getVectorNumElements();
       Base = Builder.CreateVectorSplat(NumElts, Base);
     }
     return new ICmpInst(Cond, Base,
-                        ConstantExpr::getBitCast(cast<Constant>(RHS),
-                                                 Base->getType()));
+                        ConstantExpr::getPointerBitCastOrAddrSpaceCast(
+                            cast<Constant>(RHS), Base->getType()));
   } else if (GEPOperator *GEPRHS = dyn_cast<GEPOperator>(RHS)) {
     // If the base pointers are different, but the indices are the same, just
     // compare the base pointer.
     if (PtrBase != GEPRHS->getOperand(0)) {
       bool IndicesTheSame = GEPLHS->getNumOperands()==GEPRHS->getNumOperands();
       IndicesTheSame &= GEPLHS->getOperand(0)->getType() ==
                         GEPRHS->getOperand(0)->getType();
       if (IndicesTheSame)

test/Transforms/InstCombine/gep-inbounds-null.ll

 ; CHECK-NEXT:    [[CND:%.*]] = icmp eq i8 addrspace(2)* [[GEP]], null
 ; CHECK-NEXT:    ret i1 [[CND]]
 ;
 entry:
   %gep = getelementptr inbounds i8, i8 addrspace(2)* %base, i64 %idx
   %cnd = icmp eq i8 addrspace(2)* %gep, null
   ret i1 %cnd
 }
+
+; Test for an assert from trying to create an invalid constantexpr
+; bitcast between different address spaces. The addrspacecast is
+; stripped off and the addrspace(0) null can be treated as invalid.
+; FIXME: This should be able to fold to ret i1 false
+define i1 @invalid_bitcast_icmp_addrspacecast_as0_null(i32 addrspace(5)* %ptr) {
+; CHECK-LABEL: @invalid_bitcast_icmp_addrspacecast_as0_null(
+; CHECK-NEXT:  bb:
+; CHECK-NEXT:    [[TMP2:%.*]] = icmp eq i32 addrspace(5)* [[PTR:%.*]], addrspacecast (i32* null to i32 addrspace(5)*)
+; CHECK-NEXT:    ret i1 [[TMP2]]
+;
+bb:
+  %tmp1 = getelementptr inbounds i32, i32 addrspace(5)* %ptr, i32 1
+  %tmp2 = icmp eq i32 addrspace(5)* %tmp1, addrspacecast (i32* null to i32 addrspace(5)*)
+  ret i1 %tmp2
+}
+
+define i1 @invalid_bitcast_icmp_addrspacecast_as0_null_var(i32 addrspace(5)* %ptr, i32 %idx) {
+; CHECK-LABEL: @invalid_bitcast_icmp_addrspacecast_as0_null_var(
+; CHECK-NEXT:  bb:
+; CHECK-NEXT:    [[TMP2:%.*]] = icmp eq i32 addrspace(5)* [[PTR:%.*]], addrspacecast (i32* null to i32 addrspace(5)*)
+; CHECK-NEXT:    ret i1 [[TMP2]]
+;
+bb:
+  %tmp1 = getelementptr inbounds i32, i32 addrspace(5)* %ptr, i32 %idx
+  %tmp2 = icmp eq i32 addrspace(5)* %tmp1, addrspacecast (i32* null to i32 addrspace(5)*)
+  ret i1 %tmp2
+}