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

[Stefanos Baziotis via Phabricator via llvm-commits]

baziotis marked an inline comment as done.
baziotis added inline comments.


================
Comment at: llvm/test/Transforms/Attributor/IPConstantProp/PR26044.ll:9
+;    live value is from %entry. Right now, AAIsDead does not
+;    interact with AAUB to know about the dead for.cond1.
 define void @fn2(i32* %P) {
----------------
baziotis wrote:
> jdoerfert wrote:
> > baziotis wrote:
> > > jdoerfert wrote:
> > > > This test is actually really broken. The function can never be executed w/o causing UB. We need to fix these tests to have some behavior. 
> > > Did I break anything? As far as I could understand, this was true before as well.
> > > Or just a general comment? I personally think that now we have `AAUB`, it's ok to have "always UB" functions on tests that are supposed to test UB (like in the `undefined_behavior.ll`) but here it may be irrelevant to always have UB.
> > The test was broken, the new AAUB just exposed it (more and more). Since this is supposed to test AAValueSimplify, we should remove the UB. Can you replace the undef in the branch with some argument value (which you need to add), and the undef in the phi with %p. Then let the jump go to some exit block that returns. At least that way we have defined behavior until the Attributor gets smart enough to realize the endless loop has no atomic side effect and can be removed as UB as well ;)
> If I understood correctly, the code will look like this:
> ```
> entry:
>   br label %if.end
> 
> for.cond1:
>   br i1 %C, label %if.end, label %if.end
> 
> if.end:
>   %e.2 = phi i32* [ %P, %entry ], [ null, %for.cond1 ], [ null, %for.cond1 ]
>   %0 = load i32, i32* %e.2, align 4
>   ...
>   br label %exit
> exit:
>   ret void
> }
> ```
> where `%C` is an argument. If we do that, there's no endless loop and the `%for.cond1` branch is unreachable (not because AAUB, but because there's no way to reach it).
> Then let the jump go to some exit block that returns

I had forgotten about this. Sorry, just yesterday I learned about the terminology of loops. So I didn't get that "exit block" is a characteristic term that makes clear that you were referring to the "else" jump on the conditional branch and not the jump on the latch.
It should be ok now.


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

https://reviews.llvm.org/D71960