[PATCH] D90529: Allow nonnull attribute to accept poison

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added subscribers: okura, reames.
jdoerfert added a comment.

In D90529#2369382 <https://reviews.llvm.org/D90529#2369382>, @aqjune wrote:

> In D90529#2368303 <https://reviews.llvm.org/D90529#2368303>, @jdoerfert wrote:
>
>> Fair. Though, I think we want to produce poison for one set of attributes for which the name "value attribute" was not well chosen.
>> So far, the things I think should produce poison not UB are:
>>
>> (pure) value attributes:
>>
>> - nonnull
>> - align
>> - [used_bits] (not existing yet)
>>
>> (context) value attributes:
>>
>> - dereferenceable
>> - dereferenceable_or_null
>> - [object_size] (as proposed on the list)
>>
>> WDYT?
>
> In case of `dereferenceable`, my opinion is still slightly different: it represents the property of the memory at the point.
> If a memory block is freed, a same pointer value won't be dereferenceable after the deallocation.

That is why it is not listed in the "pure" category ;). It is not only a property of the value, but it still should be split wrt UB behavior. See below for my reasoning based on your example.

Partially related:
It is, nowadays, unclear to me if your interpretation of dereferenceable is what we want though. It is/was my interpretation as well FWIW.
@reames suggested that dereferenceability is not about it pointing to an allocated object but to a memory location that can be loaded without causing observable behavior, i.a., a trap.
Unsure if we want a new attribute for that but certainly there are reasons that you want that information which is a "value property" and not tied to the "allocation status".

> Other than the conceptual reason, my practical concern about `dereferenceable` is that `dereferenceable` is hard to use unless it is with `noundef`.

Regardless of my argument below, nobody said you cannot use `dereferenceable` with `noundef` "all the time".
I mean, if you argue a current use of `dereferenceable` would actually require the "UB if violated" behavior, then simply also add `noundef` as well.

> Since loading poison is UB, the attribute can't still give a guarantee that loading the pointer is well defined.

Can you clarify "loading poison is UB". I'm unsure I follow/agree.

FWIW, loading a dereferenceable pointer might result in poison but it's even not clear to me that out-of-bounds accesses are UB as of now (in IR).
Maybe I just forgot where it says I cannot do `load i32, i32* (bitcast i8* [alloca i8] to i32*)`. I certainly would get 3 poison bytes if not UB.

> Furthermore, `noundef` is hard to infer from the context. For example,
>
>   store i32 0, i32* %p
>   call void @f(i32* %p) ; can we infer %p's noundef & dereferenceable(4) from store?
>
> `store i32 0, i32* %p` does not guarantee that `%p` is noundef because it can be partially undef but still dereferenceable (D87994 <https://reviews.llvm.org/D87994> is a relevant LangRef patch).
>
> The example above was propagating the attribute from caller to callee, but the inverse direction (callee -> caller) is the same: If `@f` contains a store, it guarantees that the pointer is dereferenceable but not noundef.

OK, I'm unsure I get the point though. We can derive `dereferenceable` in either case, agreed? What is currently better?

(@hideto @stefan1 @okura, I think the Attributor might derive noundef here but should not, wdyt?)

> In order to regain optimization power, we need a way to attach noundef more aggressively.

This is correct but orthogonal. Having `noundef` "backed-in" to other attributes is not helping us to derive `noundef` it just prevents us to derive other attributes.

> D81678 <https://reviews.llvm.org/D81678> will attach noundef to arguments, and additionally I think it is valid for clang to attach `!noundef` when lowering non-char typed lvalues to load.

Great.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90529