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

[Hideto Ueno via Phabricator via llvm-commits]

uenoku added a comment.

In D71799#1794247 <https://reviews.llvm.org/D71799#1794247>, @baziotis wrote:

> 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).


  bool isAssumedToCauseUB(I*){
     return !NoUBInsts.count(I);
  }
  ...
  SimplifiedV = ...;
  if (!SimplifiedV.hasValue())
    // Do nothing. Assumption holds (Because the value might be simplified to `undef`)
  else {
    Val = SimplifiedV.getValue();
    if (Val is undef)
      // Do nothing. Assumption holds.
    else
      NoUBInsts.insert;
  }

I think this will work. It is no problem to have both 2 sets (but a bit redundant). If so,

  SimplifiedV = ...;
  if (!SimplifiedV.hasValue())
    // Assumption holds (Because the value might be simplified to `undef`)
    UBInsts.insert
  else {
    Val = SimplifiedV.getValue();
    if (Val is undef)
      //Assumption holds.
      UBInsts.insert
    else
      NoUBInsts.insert;
      UBInsts.remove;
  }


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

https://reviews.llvm.org/D71799