[PATCH] D90529: Allow nonnull/align attribute to accept poison

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

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

> In D90529#2596260 <https://reviews.llvm.org/D90529#2596260>, @jdoerfert wrote:
>
>> In D90529#2596245 <https://reviews.llvm.org/D90529#2596245>, @aqjune wrote:
>>
>>>   call void @llvm.assume(true) ["attr1"(p)]
>>>   call void @llvm.assume(true) ["attr2"(p)]
>>>     =>
>>>   call void @llvm.assume(true) ["attr1"(p), "attr2"(p)] ; synergy between attributes may introduce UB
>>>
>>> TBH, making things simple and introducing no special rule seems to be the best practice for avoiding possible miscompilations or glitches in spec I think.
>>> I see that a similar thing is happening in relaxed concurrency and it is really hard to write at least medium-scale program correctly without relying on commonly-used patterns. :(
>>
>> I think I missed your "simple" no-special rule suggestion. Could you repeat that or link it?
>
> My suggestion is to keep the LangRef text of assume bundle as it is, and add nonnull/align to a bundle when creating assume only if it was paired with noundef.
>
>   call void @f(nonnull %p)         ; => llvm.assume(true) []
>   call void @f(nonnull noundef %p) ; => llvm.assume(true) ["nonnull"(%p)]
>
> Well, it is still doing something (the lowering needs to check the existence of noundef), but I think the principle is already applied to the nonnull/align attributes; they can raise UB only if it is paired with noundef, as LangRef says.

I still like this, it seems like a natural next step, let's do that first.
We can revisit the scope discussion and I think will have to because of the GVN example, among other things.

> I'm sorry if it sounded a bit aggressive :(

No worries.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90529