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

[Stefanos Baziotis via Phabricator via llvm-commits]

baziotis added a comment.

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

> In D71799#1794262 <https://reviews.llvm.org/D71799#1794262>, @baziotis wrote:
>
> > Note that while this is what makes sense to me, Johannes told me that if `SimplifiedV` gives `None` (i.e. it doesn't have value), then we can assume
> >  that it is `undef` but I don't know why this is true.
>
>
> A state of `SimplifiedV` is `Optional<Value*>` representing its simplified associated value `V`. Initially, set to `None`. If a candidate `V1` is found, it is set to `Some(V1)`. If another candidate `V2` is found, trying to unify `V1` and `V2`. 
>  When the simplified value is `None`, we haven't found a candidate yet, or there is no candidate(like `int f(x) { return f(x);}`). So you can choose any arbitrary value(=undef) in that assumption.


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.

>> Edit: Just saw your comment, which you seem to not assume that.
>>  In your code, why insert it in `UBInsts` and then remove it? Since we query `AAValueSimplify`, isn't that going to call us again and thus at some point get the value (which in turn means we don't insert it and just wait for when we'll be called again).
> 
> The simplified value may change (None -> concrete value) so we need to track.

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.
But now I realize that we have to add it since e.g. `isAssumedToCauseUB()` and stats depend on it. So, I think your version with only `NoUBInsts` set is better with an additional `UBInstsSize` (for stats).


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

https://reviews.llvm.org/D71799