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

Tue Dec 31 09:05:56 PST 2019

baziotis marked an inline comment as done.
baziotis added inline comments.

================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:3120
+    }
+    AliveSuccessors = std::move(AliveSuccessorsCopy);
+
----------------
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.

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

https://reviews.llvm.org/D71974