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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert accepted this revision.
jdoerfert added a comment.
This revision is now accepted and ready to land.

In D68706#1701937 <https://reviews.llvm.org/D68706#1701937>, @spatel wrote:

> In D68706#1701915 <https://reviews.llvm.org/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)



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp:2349
+        // In a non-default address space (not 0), a null pointer can not be
+        // assumed inbounds, so ignore that case (dereferenceable_or_null).
+        if (SrcPTy->getAddressSpace() == 0 || !CanBeNull)
----------------
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.
```


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

https://reviews.llvm.org/D68706