[PATCH] D47441: SafepointIRVerifier should ignore dead blocks and dead edges

[Artur Pilipenko via Phabricator via llvm-commits]

apilipenko accepted this revision.
apilipenko added inline comments.


================
Comment at: lib/IR/CFGDeadness.cpp:12
+// of blocks unreachable from entry then propagates deadness using foldable
+// conditional branches like GVN does but without splitting critical edges.
+// So the CFG is kept intact.
----------------
I don't think that GVN reference is important here. It just happened to be the detail of the current implementation. I think the higher level interface/comment should be that the analysis tracks trivially dead edges without modifying the CFG.

Could you please add a comment that in most cases passes rely on SimplifyCFG to clean up such blocks, but in some cases, like verification or loop passes it's not possible. 


================
Comment at: lib/IR/SafepointIRVerifier.cpp:242
+/// without splitting critical edges. So the CFG is kept intact.
+class DeadBlocks : public SetVector<const BasicBlock *> {
+  const DominatorTree &DT;
----------------
anna wrote:
> apilipenko wrote:
> > anna wrote:
> > > This seems like a generally useful analysis to have outside of the safepoint IR Verifier. We should think of moving the code into Analysis. 
> > > 
> > > You mentioned it's taken from the GVN but modified to avoid splitting the critical edges. Could you please state that as a comment here: modified version from `GVN::addDeadBlock`. 
> > > 
> > @anna, I think this is not of much use outside of SafepointIRVerifier. Most passes don't need to worry about this patterns as it's reasonable to expect simplifycfg to have cleaned things up.
> > 
> > Verifiers is a different story as you don't want you verification logic depend on the optimizations performed on the code. You also don't want to modify the code to clean up the mess.
> > 
> > I would suggest keeping this logic in SafepointIRVerifier. If we want to extract it as a separate analysis I'd suggest making a clear comment that it has very specific purpose.
> There's one specific use for this pass outside of the SafepointIRVerifier and that's for loopSimplifyCFG - we cannot rely on SimplifyCFG to cleanup dead/unreachable blocks that are dead *because* of LoopSimplifyCFG. So, I suspect this will be useful for loop optimizations that are run as a group in a pipeline without the need for a function pass like `SimplifyCFG`. 
> 
> Perhaps we can add a clear comment stating where it's usage is expected and that right now it's used in the SafepointIRVerifier.
> 
> 
Makes sense.


https://reviews.llvm.org/D47441