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

[Stefanos Baziotis via Phabricator via llvm-commits]

baziotis added a comment.

In D71799#1794216 <https://reviews.llvm.org/D71799#1794216>, @uenoku wrote:

> In the current patch, I think an instruction might be inserted to both `UBInsts` and `NoUBInsts`  in some cases when `AAValueSimplify` changes its assumption.


Yes, I forgot to add the check that guards this the previous patches (check the first `if` in `InspectMemAccessInstForUB`).

> In my understanding, the idea of this deduction is 
>  " We assume `br` instruction causes UB. If you can prove that the instruction *doesn't* cause UB, we remove that assumption".

Yes exactly. Well, my initial assumption was this:

  SimplifiedV = ...;
  if (!SimplifiedV.hasValue())
    ... // Do nothing for now, no value yet.
  else {
    Val = SimplifiedV.getValue();
    if (Val is undef)
      UBInsts.insert;
    else
      NoUBInsts.insert;
  }

Then, now that I see that again apparently I got confused somewhere so let me change this quickly. :P
Is this what you had in mind ?

> So I think we don't need to have both `UBInsts` and `NoUBInsts` because if an instruction is not in `NoUBInsts` then it is assumed to cause UB. 
>  ( You can find that `AAISDeadFunction` is similar. If `BB` is not in `AssumedLiveBlocks`, then it is assumed to be Dead`).

As I noted above, these 2 sets should have no common elements. With that, having both of them just makes some things easier
(like stats, looping over UB instructions in manifest, checking if an instruction is UB).


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

https://reviews.llvm.org/D71799