This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine] don't assume 'inbounds' for bitcast pointer to GEP transform (PR43501)
ClosedPublic

Authored by spatel on Sep 30 2019, 1:59 PM.

Details

Summary

I don't know the overall impact of this change, or if it goes far enough, but based on:
https://bugs.llvm.org/show_bug.cgi?id=43501
...we can't declare the GEP 'inbounds' in general. But we can salvage that information if we have known dereferenceable bytes on the source pointer.

Diff Detail

Event Timeline

spatel created this revision.Sep 30 2019, 1:59 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 30 2019, 1:59 PM
spatel updated this revision to Diff 222827.Oct 2 2019, 7:01 AM
spatel edited the summary of this revision. (Show Details)
spatel added a reviewer: jdoerfert.

Patch updated:
Use 'dereferenceable' knowledge to deduce 'inbounds'. This removes the 3 diffs from the previous revision that had an argument with dereferenceable bytes.

lebedev.ri added inline comments.
llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp
2344–2345

Why beyond? You check >=.

2351

@jdoerfert @uenoku @sstefan1
attributor potential missing feature: i'd expect that it could set this inbounds, but it does not: https://godbolt.org/z/zkUt1-

spatel marked 2 inline comments as done.Oct 2 2019, 7:34 AM
spatel added inline comments.
llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp
2344–2345

Yes, inaccurate comment. I'll fix it...

2351

Right...so I proposed this change based on the comments in PR43501, but it raises a question:
Do we want instcombine doing these limited improvements, or is it better to leave that entire responsibility to Attributor?

lebedev.ri added inline comments.Oct 2 2019, 7:38 AM
llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp
2351

Once attributor is enabled and can do this, if it will run frequent-enough in the pipeline,
i'd strongly insist that it should do this.

But currently it does not, so let's keep this for now.

spatel updated this revision to Diff 222837.Oct 2 2019, 7:53 AM

Patch updated:
Change code comment to match logic.

spatel marked an inline comment as done.Oct 2 2019, 7:54 AM
lebedev.ri added inline comments.Oct 2 2019, 8:01 AM
llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp
2348
/// Note that this may not reflect the size of memory allocated for an
/// instance of the type or the number of bytes that are written when an
/// instance of the type is stored to memory. The DataLayout class provides
/// additional query functions to provide this information.
///
unsigned getPrimitiveSizeInBits() const LLVM_READONLY;

We are sure this is what we want?

jdoerfert added inline comments.Oct 2 2019, 9:02 AM
llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp
2351

Why all this logic? bitcast -> gep inbounds 0, 0 should be fine as long as the pointer is null or points into (or to the end of) an allocated object.

@lebedev.ri
We can add an attribute to add inbounds to GEPs if we determine the above condition easily, I'll keep you posted. Open a bug if you want to make sure ;)

lebedev.ri added inline comments.Oct 2 2019, 9:42 AM
llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp
2351

Why all this logic? bitcast -> gep inbounds 0, 0 should be fine as long as the pointer is null or points into (or to the end of) an allocated object.

We don't know that the input is inbounds, see https://bugs.llvm.org/show_bug.cgi?id=43501
Or is there any such requirement for bitcast?

lebedev.ri added inline comments.Oct 2 2019, 10:00 AM
llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp
2348

Actually, what @jdoerfert was saying is that we don't care what the size of the type is.
We only need to check that we have at least one dereferenceable byte.

jdoerfert added inline comments.Oct 2 2019, 10:12 AM
llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp
2348

That is what I should have said at least ;)

spatel updated this revision to Diff 222888.Oct 2 2019, 12:05 PM

Patch updated:
Loosen the dereferenceable constraint - any deref will do. Add a test to show that.

lebedev.ri accepted this revision.Oct 2 2019, 12:12 PM

LG now.

This revision is now accepted and ready to land.Oct 2 2019, 12:12 PM
jdoerfert added a comment.EditedOct 2 2019, 1:47 PM

You accidentally or intentionally did the right thing when it comes to deref_or_null.
That is,

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

should also work fine.

spatel added a comment.Oct 3 2019, 4:27 AM

You accidentally or intentionally did the right thing when it comes to deref_or_null.
That is,

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

should also work fine.

Thanks, I'll add that test too. It was intentional to allow that possibility based on the previous comments, but admittedly, I'm not very familiar with 'inbounds' semantics/usage. The LangRef says:
"The only in bounds address for a null pointer in the default address-space is the null pointer itself."
So what does that mean for a null pointer *not* in the default address-space?

This revision was automatically updated to reflect the committed changes.