Page MenuHomePhabricator

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

Authored by spatel on Wed, Oct 9, 8:14 AM.

Details

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.

Diff Detail

Event Timeline

spatel created this revision.Wed, Oct 9, 8:14 AM
Herald added a project: Restricted Project. · View Herald TranscriptWed, Oct 9, 8:14 AM

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

jdoerfert added a comment.EditedWed, Oct 9, 9:49 AM

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 updated this revision to Diff 224108.Wed, Oct 9, 11:20 AM

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.

Can you add the test I provided as well?

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 accepted this revision.Wed, Oct 9, 12:59 PM

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.
This revision is now accepted and ready to land.Wed, Oct 9, 12:59 PM
This revision was automatically updated to reflect the committed changes.