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

[Stefanos Baziotis via Phabricator via llvm-commits]

baziotis added a comment.

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

> > I agree but I assumed wrongly apparently. You see, with "we can assume it is undef" I thought that I should also add it to the UBInsts set. Up to that point, if I were to do that, it would be wrong since
> >  if something is added to UBInsts, it would never be checked again. But apparently Johannes did not mean this.
>
> I got the point! I missed that UB is intended to be used for liveness. Sorry about that. I'll rethink the problem. But it seems for me that current implementation regards *assumed* `UBInsts` as *known* `UBInsts`. Because once `I` is assumed to have UB, we never visit `I`. I guess this will cause invalid deduction.
>
> > I agree yes. My point was: At some point, we _will_ get a concrete value (or, candidate), aren't we? If so, why add it and then remove it from the set instead of just waiting until we get a value? Hence my initial code.
>
> I think we won't get a concrete value for `None` in the iterations.


Well, actually //I// am sorry for that. The reason I haven't uploaded a diff yet is that I came across what you just said. Basically, I changed the code to keep 2 sets, one for `KnownNoUBInsts` and another for `KnownUBInsts`. With that the code becomes quite better as we can do:

  if (!SimplifiedV.hasValue()) {
    // No value yet, we can assume any value: assume this is undef BUT
    // this is not _known_ so we don't put in the known set.
  } else {
    if (undef) {
      // insert in KnownUB
    } else {
      // insert in NoUB.
    }
  }

That is better because:
a) We can use the `KnownUB` set for the stats
b) We can use `KnownNoUB` set for the `isAssumedToCauseUB`.

But I can't progress because as you said, for some reason we never get a concrete value in the iterations. So, in the manifest there are 2 cases:

1. make unreachable only those in `KnownUB`. The problem with that is that exactly because we don't get a concrete value, in something like this:

  define i1 @ret_undef() {
    ret i1 undef
  }
  
  define void @test() {
    %cond = call i1 @ret_undef()
    br i1 %cond, ...

the branch never makes it to the `KnownUB`.

2. Make unreachable any instruction that `isAssumedToCauseUB`. Those are all the instructions that are not in `KnownNoUB`. That apart from the fact that it doesn't seem all that correct,

it also causes stack dumps. :P


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

https://reviews.llvm.org/D71799