[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>>,
----------------
jdoerfert wrote:
> 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.
Found that actually LowerMemIntrinsics.cpp was also checking the size rather than comparing & incrementing pointers (llvm/test/CodeGen/NVPTX/lower-aggr-copies.ll ) . So it was UB-safe.
Though, I wonder why it isn't directly emitting the increment-of-pointer version, which seems to be more efficient. Another concern is the safety of LSR that seems to be related with this as well.

I'm fine with removing noundef from memset/cpy/move (which is the current version).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86576