[PATCH] D71974: [Attributor][WIP] Connect AAIsDead with AAUndefinedBehavior

[Stefanos Baziotis via Phabricator via llvm-commits]

baziotis marked 3 inline comments as done.
baziotis added a comment.

In D71974#1908675 <https://reviews.llvm.org/D71974#1908675>, @jdoerfert wrote:

> Tests?


A lot of tests fail but I'm not sure if it makes sense to include any tests that test the new behavior yet. Do you want invalid tests?
For example, this now:

  define i32 @example(i1* %alloc) {
    %cond = load i1, i1* %alloc
    br i1 %cond, label %t, label %e
  t:
    ret i32 1
  e:
    ret i32 2
  }

results in:

  define i32 @example(i1* nocapture nofree nonnull readnone dereferenceable(1) %alloc) #0 {
    br i1 undef, label %t, label %e
  
  t:                                                ; preds = %0
    unreachable
  
  e:                                                ; preds = %0
    unreachable
  }

And again, as far as I can understand, removing something from alive successors means it isn't getting updated by the AAUB because AAUB never sees it (as it is dead).
Honestly, unfortunately I can't see how this connection fits into the whole picture given the current structure.



================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2285
     for (Instruction *I : KnownUBInsts)
-      A.changeToUnreachableAfterManifest(I);
+      //A.changeToUnreachableAfterManifest(I);
     return ChangeStatus::CHANGED;
----------------
jdoerfert wrote:
> What is happening here? Leftover?
No, I think a previous commit states that I put that to test only what AAIsDead deletes. I removed it now.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2322
 private:
+  MustBeExecutedContextExplorer *Explorer;
   /// A set of all the (live) instructions that are assumed to _not_ cause UB.
----------------
jdoerfert wrote:
> I would prefer if we pass the Explorer (or the Attributor) instead of caching it.
Yes sorry, that was a leftover.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:3418
+        const BasicBlock *BB = AliveSuccessor->getParent();
+        AssumedLiveBlocks.erase(BB);
+      } else {
----------------
jdoerfert wrote:
> I don't think retroactively removing blocks here is a good idea. Once assumed live it should stay that way.
I agree, as I had stated in a previous comment:
> (also maybe the fact that we're removing - and it's the only place - messes with the monotony).
which as far as I can understand, still holds.


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

https://reviews.llvm.org/D71974