[PATCH] D90529: Allow nonnull attribute to accept poison

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

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

> In D90529#2366723 <https://reviews.llvm.org/D90529#2366723>, @jdoerfert wrote:
>
>> I think we have two choices here:
>>
>> 1. Don't raise UB when "value attributes" are passed a "wrong value", e.g., `null` for a `nonnull` attribute, but make the value poison. Use `nonull` + `noundef` to make it UB.
>> 2. Make all "value attributes" accept poison without raising UB.
>>
>> If we don't do 1), we should talk about 2) before we make `nonnull` special.
>
> It seems these two choices are entangled.
> If `f(nonnull poison)` is okay (in other words, not UB), then `f(nonnull null)` shouldn't be UB as well.
> The reason is that `poison` can be folded into `null` in any time.
> For example, when `inbounds` is stripped from `v = gep inbounds ...`, `v` can be transformed from `poison` to `null` pointer.

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 ;)

>> I fail to see the reason it is different from any other (or at least most) "value attributes".
>
> I had a thought about this, and here's what I think:
> Each value attribute has a primary transformation to target.
> For example, `nonnull` is for null comparison folding, and `dereferenceable` is for reordering of load instructions by guaranteeing that dereferencing the pointer never raises UB (unless the pointer is freed).
>
> I think `nonnull` is different from `dereferenceable` in that allowing it to be poison doesn't break the intended optimization.
> This folding is still okay even if the pointer is poison.
> By allowing poison, further optimizations can be done as well because `gep inbounds ptr, 1` can be marked as nonnull.
> However, allowing `dereferenceable` to be `poison` will block such optimizations, which doesn't seem quite desirable IMO.
>
> This causes the properties of attributes slightly heterogeneous, but I think it is fine if the goal is to support more optimizations.

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.

What is the reason not to go with 1) above, so to remove the UB behavior and produce poison. We write an RFC and do the change.
FWIW, that is what AAUndefinedBehavior in the Attributor already does, only with `noundef` it will deduce UB for `nonnull null`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90529