[PATCH] D86576: [IR] Add NoUndef attribute to Intrinsics.td

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
Comment at: llvm/include/llvm/IR/Intrinsics.td:557
                              NoAlias<ArgIndex<0>>, NoAlias<ArgIndex<1>>,
+                             NoUndef<ArgIndex<0>>, NoUndef<ArgIndex<1>>,
                              WriteOnly<ArgIndex<0>>, ReadOnly<ArgIndex<1>>,
----------------
aqjune wrote:
> aqjune wrote:
> > jdoerfert wrote:
> > > This is the old `nonnull` discussion all over again. I don't think you can do this here. Do it when we know the size is not 0. Same below.
> > I think we can make it more explicit by updating LangRef and saying that undef or poison can be given when the size is zero. I'll make a patch for that.
> I became to think that it has to be UB, because it makes expanding memset/memcpy to a loop unsound. For example:
> ```
> memset(p, 255, 0)
> ->
> q = p+0
> while (p != q) { *p = 255; ++p; }
> ```
> If p is undef/poison, `p != q` is also undef/poison, so the target has branch on undef/poison, which is UB.
> In order to support this, `memset(undef, 255, 0)` should be UB too. LowerMemIntrinsics.cpp does this transformation.
> 
> This does not happen when p was null or dangling pointer (which is weaker than undef/poison);  pointer comparison does not produce poison in these case.
> The original thread http://lists.llvm.org/pipermail/llvm-dev/2017-July/115665.html states the cases that are only about null/dangling pointers.
> 
> I'll see whether it is okay to be UB by running Alive2.
I am hesitant to argue there is an implementation for which this is UB so we make it UB.

```
memset(p, c, size) {
  while (size > 0) {
    p[size--] = c;
  }
}
```
is UB free for size 0.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86576