[InstCombine] don't assume 'inbounds' for bitcast deref or null pointer in non-default address space

[spatel]

Summary

Follow-up to D68244 to account for a corner case discussed in:
https://bugs.llvm.org/show_bug.cgi?id=43501

Add one more restriction: if the pointer is deref-or-null and in a non-default (non-zero) address space, we can't assume inbounds.

[lebedev.ri]

You want llvm::NullPointerIsDefined(), which also checks for "null-pointer-is-valid" attribute.

[jdoerfert]

In D68706#1701551, @lebedev.ri wrote:

You want llvm::NullPointerIsDefined(), which also checks for "null-pointer-is-valid" attribute.

getPointerDereferenceableBytes should do the above. The tests in the file show it works except there is one missing:

define float @matching_scalar_smallest_deref_addrspace(<4 x float> addrspace(4)* dereferenceable(1) %p) {
; CHECK-LABEL: @matching_scalar_smallest_deref_addrspace(
; CHECK-NEXT:    [[BC:%.*]] = getelementptr inbounds <4 x float>, <4 x float> addrspace(4)* [[P:%.*]], i64 0, i64 0
; CHECK-NEXT:    [[R:%.*]] = load float, float addrspace(4)* [[BC]], align 16
; CHECK-NEXT:    ret float [[R]]
;
  %bc = bitcast <4 x float> addrspace(4)* %p to float addrspace(4)*
  %r = load float, float addrspace(4)* %bc, align 16
  ret float %r
}

site note: The CanBeNull is commonly used as _or_null flag, not to indicate the actual null pointer. That is why the test should result in the inbounds even though the pointer "can be null" but it is for sure "deref".

I think this is fine but I want to hear if people agree.

[spatel]

Patch updated:
No diffs in this patch itself, but rebased after adding test at rL374190.
I don't have much experience with addrspaces or inbounds, so let me know if I should change anything else.

[jdoerfert]

Can you add the test I provided as well?

[spatel]

In D68706#1701915, @jdoerfert wrote:

Can you add the test I provided as well?

Did I miss a message? I copy/pasted at line 92 of the test file (no diff from the code change):

define float @matching_scalar_smallest_deref_addrspace(<4 x float> addrspace(4)* dereferenceable(1) %p) {
; CHECK-LABEL: @matching_scalar_smallest_deref_addrspace(
; CHECK-NEXT:    [[BC:%.*]] = getelementptr inbounds <4 x float>, <4 x float> addrspace(4)* [[P:%.*]], i64 0, i64 0
; CHECK-NEXT:    [[R:%.*]] = load float, float addrspace(4)* [[BC]], align 16
; CHECK-NEXT:    ret float [[R]]
;
  %bc = bitcast <4 x float> addrspace(4)* %p to float addrspace(4)*
  %r = load float, float addrspace(4)* %bc, align 16
  ret float %r
}

[jdoerfert]

In D68706#1701937, @spatel wrote:

In D68706#1701915, @jdoerfert wrote:

Can you add the test I provided as well?

Did I miss a message? I copy/pasted at line 92 of the test file (no diff from the code change):

define float @matching_scalar_smallest_deref_addrspace(<4 x float> addrspace(4)* dereferenceable(1) %p) {
; CHECK-LABEL: @matching_scalar_smallest_deref_addrspace(
; CHECK-NEXT:    [[BC:%.*]] = getelementptr inbounds <4 x float>, <4 x float> addrspace(4)* [[P:%.*]], i64 0, i64 0
; CHECK-NEXT:    [[R:%.*]] = load float, float addrspace(4)* [[BC]], align 16
; CHECK-NEXT:    ret float [[R]]
;
  %bc = bitcast <4 x float> addrspace(4)* %p to float addrspace(4)*
  %r = load float, float addrspace(4)* %bc, align 16
  ret float %r
}

I did not realize you comited the test separately so I was expecting to see a new test show in the diff. Sorry.

LGTM with a request for an additional comment (see below)

llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp

2349

Could you add one more sentence here please to complement your reasoning:

// The reason is that `null` is not treated differently in these address spaces
// and we consequently ignore the `gep inbounds` special case for `null` which
// allows `inbounds` on `null` if the indices are zeros.

llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp

if (PointerType *DstPTy = dyn_cast<PointerType>(DestTy)) {
     if (SrcElTy == DstElTy) {
       SmallVector<Value *, 8> Idxs(NumZeros + 1, Builder.getInt32(0));
       GetElementPtrInst *GEP =
           GetElementPtrInst::Create(SrcPTy->getElementType(), Src, Idxs);
 
       // If the source pointer is dereferenceable, then assume it points to an
       // allocated object and apply "inbounds" to the GEP.
       bool CanBeNull;
-      if (Src->getPointerDereferenceableBytes(DL, CanBeNull))
+      if (Src->getPointerDereferenceableBytes(DL, CanBeNull)) {
+        // In a non-default address space (not 0), a null pointer can not be
+        // assumed inbounds, so ignore that case (dereferenceable_or_null).
+        // The reason is that 'null' is not treated differently in these address
+        // spaces, and we consequently ignore the 'gep inbounds' special case
+        // for 'null' which allows 'inbounds' on 'null' if the indices are
+        // zeros.
+        if (SrcPTy->getAddressSpace() == 0 || !CanBeNull)
           GEP->setIsInBounds();
+      }
       return GEP;
     }
   }
 
   if (VectorType *DestVTy = dyn_cast<VectorType>(DestTy)) {
     if (DestVTy->getNumElements() == 1 && !SrcTy->isVectorTy()) {
       Value *Elem = Builder.CreateBitCast(Src, DestVTy->getElementType());
       return InsertElementInst::Create(UndefValue::get(DestTy), Elem,

llvm/test/Transforms/InstCombine/load-bitcast-vec.ll

 ; CHECK-NEXT:    [[R:%.*]] = load float, float addrspace(4)* [[BC]], align 16
 ; CHECK-NEXT:    ret float [[R]]
 ;
   %bc = bitcast <4 x float> addrspace(4)* %p to float addrspace(4)*
   %r = load float, float addrspace(4)* %bc, align 16
   ret float %r
 }
 
-; TODO: Is a null pointer inbounds in any address space?
+; A null pointer can't be assumed inbounds in a non-default address space.
 
 define float @matching_scalar_smallest_deref_or_null_addrspace(<4 x float> addrspace(4)* dereferenceable_or_null(1) %p) {
 ; CHECK-LABEL: @matching_scalar_smallest_deref_or_null_addrspace(
-; CHECK-NEXT:    [[BC:%.*]] = getelementptr inbounds <4 x float>, <4 x float> addrspace(4)* [[P:%.*]], i64 0, i64 0
+; CHECK-NEXT:    [[BC:%.*]] = getelementptr <4 x float>, <4 x float> addrspace(4)* [[P:%.*]], i64 0, i64 0
 ; CHECK-NEXT:    [[R:%.*]] = load float, float addrspace(4)* [[BC]], align 16
 ; CHECK-NEXT:    ret float [[R]]
 ;
   %bc = bitcast <4 x float> addrspace(4)* %p to float addrspace(4)*
   %r = load float, float addrspace(4)* %bc, align 16
   ret float %r
 }