[PATCH] D90529: Allow nonnull attribute to accept poison

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

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

> In D90529#2367050 <https://reviews.llvm.org/D90529#2367050>, @jdoerfert wrote:
>
>> Agreed. So let's go with 1) and we change all the "value attribute" semantics to produce poison on violation.
>> We have noundef for the UB case, that was the reason I wanted it ;)
>
> Thank you, glad to hear that moving towards the change!

Wasn't that my position for more than a year now ;)

> In D90529#2367050 <https://reviews.llvm.org/D90529#2367050>, @jdoerfert wrote:
>
>> I don't think we should argue this way, at least it should be the last resort. An attribute describes a property, doesn't matter if it is
>> nonnull, align, or dereferenceable, it's a property of the value. Now, due to the way things are it is possible to violate
>> that property and then we need to define what is happening. If we do this on a case-by-case basis and with specific transformations
>> in mind, we will make the IR more complex, the attributes confusing, and in the end non-composeable.
>
> I think the discussion finally falls into which attribute is a value attribute and which is not.
> Value attributes should have the same semantics as you said.
> Sometimes value attribute and non-value attribute can interact with each other (e.g., `byval` implies `nonnull` unless null is a valid pointer), but such complexity should be only at there.

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?

>> FWIW, that is what AAUndefinedBehavior in the Attributor already does, only with `noundef` it will deduce UB for `nonnull null`.
>
> Aha, thanks for the info.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90529