[PATCH] D71799: [Attributor] AAUndefinedBehavior: Check for branches on undef value.

[Hideto Ueno via Phabricator via llvm-commits]

uenoku added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2089
+    A.checkForAllInstructions(InspectBrInstForUB, *this, {Instruction::Br});
+    if (PrevSize != KnownNoUBInsts.size())
       return ChangeStatus::CHANGED;
----------------
baziotis wrote:
> uenoku wrote:
> > Why don't we need to check for `KnownUBInsts`?
> Oh yes, I've forgotten about that. I should have updated it when we ended up in using only known parts.
> So, the correctness of this procedure is described in the comment of `KnownNoUBInsts`. Since the size `KnownUBInsts` is also monotonically increasing and bounded, then the "sum" of these 2 "functions" is also monotonically increasing and bounded.
> Hence, we can (and should) include that (part of this reasoning was why inserting and removing from the set was going to give us problems).
> Probably you have thought of all that but just to be sure we're on the same page. :)
Ok, thanks.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2141
 private:
-  // A set of all the (live) memory accessing instructions
-  // that are _not_ assumed to cause UB.
+  // A set of all the (live) instructions that are known to _not_ cause UB.
   //   Note: The correctness of the procedure depends on the fact that this
----------------
Could you add a comment here to say that instruction in `NoUBInst` might cause UB?


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

https://reviews.llvm.org/D71799