[PATCH] D71974: [Attributor][WIP] Connect AAIsDead with AAUndefinedBehavior

[Stefanos Baziotis via Phabricator via llvm-commits]

baziotis added a comment.

In D71974#1799138 <https://reviews.llvm.org/D71974#1799138>, @jdoerfert wrote:

> In D71974#1798878 <https://reviews.llvm.org/D71974#1798878>, @baziotis wrote:
>
> > In D71974#1798289 <https://reviews.llvm.org/D71974#1798289>, @jdoerfert wrote:
> >
> > > Similarly, in AAUB we should go through the explorer context when we add something to the knownUB set. So if `I` is knownUB, make all instructions in the must-be-executed-context of `I` knownUB, thus insert all into the set. In AAIsDead we can then simply query `isKnownUB` and that will only need to look into the set (as before).
> >
> >
> > If I understand it correctly, this will mark as UB all the instructions from the UB instruction //and forwards// (i.e. the must be executed context goes to the successors). That will probably complicate things as to see if a BB is dead, with the current code, it makes sense to see its first instruction right?
>
>
> The must-be-executed-context is not defined to only contain "successors". It might right now but that will change eventually.
>
>  ---
>
> I think we should stick to known information right now.


Ok, `isAssumedToCauseUB()` is overoptimistic right now.

> I still believe this is more complex and less general than the approach I tried to describe. Maybe the following is a good compromise in the direction I think we should going but working already right now:
> 
> 1. Since the must-be-executed-context is not collecting predecessors yet, we need to change that. The code actually exists already, it just needs to be separated from some other improvements and put for review again. I'll look into that (or @uenoku you can if you want to).

Great, I didn't know this was planned.

> 2. Collecting instructions that are executed always with one which causes UB in the "knownUB" set seems logical to me. After all, their execution is "eventually" leading to UB. Having them in the set gives us a single consistent way to check instead of iterating over must-be-executed contexts all the time. We can keep the context loop in `isKnownUB` for now but we should eventually not do that (thus we should add a TODO).

I surely agree. My comment was relative to the current `MustBeExecutedContext`. That is, since it currently only looks successors (and I didn't know it was planned to change), it would be difficult to add the right predecessors to the set when we found a UB instruction.
And that would then make more difficult the fact that to connect it with `AAIsDead`, I was looking at the start of the block (which could be predecessor of a UB instruction). Now it's clear, thanks!

> 3. During the liveness exploration we should check if the beginning of a block is known to cause UB, if so, we do not make it live. We can even check on the instruction level if we want to later. For now, we should be able to do all this in the `assumeLive` method by "not assuming it is live" if it leads to UB.

This is sort of what I was going for (if you see the `identifyAliveSuccessors` for `BranchInst`, it marks alive only BBs whose first instruction is not UB, hence the whole forwards vs backwards etc.). But as you said, I made it too complicated as instead
of modifying all `identifyAliveSuccessors()` etc. I could just put the code in `assumeLive()` which I will now do. :)

> Are no tests affected?

A lot, I'll upload in the next diff.


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

https://reviews.llvm.org/D71974