[PATCH] D71960: [Attributor] AAUndefinedBehavior: AAValueSimplify in memory accessing instructions.

[Stefanos Baziotis via Phabricator via llvm-commits]

baziotis added a comment.

In D71960#1798010 <https://reviews.llvm.org/D71960#1798010>, @uenoku wrote:

> In D71960#1797992 <https://reviews.llvm.org/D71960#1797992>, @baziotis wrote:
>
> > Now, the more interesting part is actually looking what the Attributor does. If I followed the code correctly, previously we didn't query `AAValueSimplify`.
> >  What happens if we do (and correct me if I'm wrong, it seems useful to me to know how it works) is that `%e.2` is a floating value,
> >  which calls `genericValueTraversal()`, which for the `phi` node, calls `VisitValueCB` with the 3 incoming values: `null`, `undef` and `undef`.
> >  This in turn calls `checkAndUpdate()` which ignores `UndefValue` and finalizes on `null` (but if we had other different values, this:
> >
> >   if (AccumulatedSimplifiedValue.hasValue())
> >     return AccumulatedSimplifiedValue == QueryingValueSimplified;
> >   
> >
> > in `checkAndUpdate()` would return `false` because of conflicting values, effectively indicating pessimistic fixpoint).
> >
> > Now, other than that, I guess that with this:
> >
> > > So once `AAIsDead` can get to interact with `AAUB`, the problem will be solved.
> >
> > you meant that when `AAIsDead` will interact with `AAUB`, the former will ignore the 2 `phi`s in `genericValueTraversal()` since it will consider them dead.
>
>
> Yes, exactly.


Great :)

> Btw, my understating is that `load` inst from `undef` pointer causes UB even if "null-pointer-is-valid"="true" (https://llvm.org/docs/LangRef.html#pointeraliasing). 
>  Then, I think `@fn_no_null_opt ` can be optimized aggressively, is this correct? If so, please add `FIXME`.

Oh btw, I missed the code haha:

> Hmm, actually by looking again the code, I think that null is the correct value.

It so //isn't// the correct value :P I thought the code was: `phi = [entry, null], [for.cond1, undef], [for.cond1, undef]` but it is: `phi = [entry, undef], [for.cond1, null], [for.cond1, null]`. That's why you mentioned the FIXME above. I'll add it.
And yes, what you mentioned is totally UB and the code is written in such a way as to handle it. But it doesn't, I'll add a FIXME for that as well.
It seems there's some problem in general with the propagation of values. For example, in `undefined_behavior.ll` test, if you check `load_null_propagated()` the `null` is propagated, but `AAUB` doesn't pick it up.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71960