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

[Stefanos Baziotis via Phabricator via llvm-commits]

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

In D71799#1795803 <https://reviews.llvm.org/D71799#1795803>, @jdoerfert wrote:

> I'd wait wrt. AAValueSimplify.


I guess you meant "wait to be committed" (and not wait as to not use it in the memory accessing functions yet).

> I'd do the following two first:
> 
> 1. UnreachableInst is UB (though no need to replace it in the manifest.
> 2. Given an instruction, determine if that instruction or one later that is known to be executed is known to cause UB. We then hook that up to the AAIsDead.

Regarding 2), would it suffice to: Go through the next instructions and follow (alive) branches either by taking unconditional branches, or branches that have known value (e.g. using `AAValueSimplify`) `true`.
Note: Walk them in a DFS kind of way until we find a UB instruction (or none at all).



================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2089
+    A.checkForAllInstructions(InspectBrInstForUB, *this, {Instruction::Br});
+    if (PrevSize != KnownNoUBInsts.size())
       return ChangeStatus::CHANGED;
----------------
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. :)


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

https://reviews.llvm.org/D71799