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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

(Don't wait for me to review/accept these even if I leave random comments once in a while.)



================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2000
+      // or we got back a simplified value to continue.
+      Optional<Value *> SimplifiedPtrOp = stopOnUndefOrAssumed(A, PtrOp, &I);
+      if (!SimplifiedPtrOp.hasValue())
----------------
baziotis wrote:
> jdoerfert wrote:
> > Where is `stopOnUndefOrAssumed` defined?
> Sorry, I should have mentioned in the summary that this diff is based on: https://reviews.llvm.org/D71799
> That is already accepted and will be committed so I thought it would be good to not wait for it to actually be commited.
No worries, I just missed that part of D71799. 


================
Comment at: llvm/test/Transforms/Attributor/ArgumentPromotion/2008-07-02-array-indexing.ll:12
-; CHECK-NEXT:    [[A_0:%.*]] = load i32, i32* null
-; CHECK-NEXT:    br i1 false, label [[T:%.*]], label [[F:%.*]]
 ; CHECK:       T:
----------------
We should also repair this test. pass a valid pointer into `callee`, e.g., an argument. 


================
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:
> > 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 ;)


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

https://reviews.llvm.org/D71960