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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:3120
+    }
+    AliveSuccessors = std::move(AliveSuccessorsCopy);
+
----------------
baziotis wrote:
> baziotis wrote:
> > jdoerfert wrote:
> > > I think something like `make_filter_range` from `llvm/include/llvm/ADT/STLExtras.h` could make this nicer. Or maybe we just need to put it in a helper function, or both.
> > Yes, using a helper would be better. `make_filter_range` seems cool, I didn't know it. TBH though, I think it gives less control and predictability. It's not as clear to me as in the current what happens under the hood (apart from the fact that we'll query 2 times the `AAUB`). Also, FWIW, the current will have a worst case of one allocation and one free and an average case of no allocation / free (assuming that most instructions have less than 8 successors).
> > I'll put it in a helper function and update me if you still think it's not that good and I'll change it to `make filter_range`.
> Btw, I forgot to explain the `AssumedLiveBlocks.erase(BB)` part. Basically, if an alive successor is UB and its parent BB has been put into `AssumedLiveBlocks`, then we should mark that BB dead by removing it. That however seems bad to me. Up to now, we only inserted to it and it feels that something can go badly if we start removing stuff (like, go to an endless loop or the not having a monotone procedure).
> I'll take a look again, it was a quick change but feel free to update me if we should worry about that.
I haven't read the code in detail, some observations:

```
 if (AAUB.isKnownToCauseUB(AliveSuccessor) ||
          (Assumed = AAUB.isAssumedToCauseUB(AliveSuccessor))) {
```
Assumed is always true or uninitialized here. Reading it after results in true or UB. What you want is to check assumed and if true check known to determine if you used assumed information.

---

No need for `count` if you call `erase`. Just erase/insert stuff, the return value even tells you if it was in before.

---

I would like to understand the situation in which we think a block is UB *only after* it was put in `AssumedLiveBlocks`. We should not erase it but add an assertion so we can see if it triggers on the test. I hope it does not (and we can keep the assertion) as that would mean it works properly.


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

https://reviews.llvm.org/D71974