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

[Juneyoung Lee via Phabricator via llvm-commits]

aqjune 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:
> 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86576