[PATCH] D157621: [CHR] Fix up phi nodes with unreachable predecessors (PR64594)

[Eli Friedman via Phabricator via llvm-commits]

efriedma added a comment.

I can't think of any better way to handle this overall, given the way regions and dead basic blocks work.

Stuff like this makes me tempted to say we should try to forbid unreachable blocks in IR; it's not really that much simpler overall, but the complexity would be distributed in a way that's less likely to lead to rare crashes.



================
Comment at: llvm/lib/Transforms/Instrumentation/ControlHeightReduction.cpp:1786
+      SmallVector<BasicBlock *> DeadPreds;
+      for (PHINode &PN : NewBB->phis()) {
+        for (BasicBlock *Pred : PN.blocks())
----------------
I think `if (!NewBB->phis().empty())` or something like would be more readable?  It's sort of confusing to have a for loop with only one iteration.

Is iterator invalidation actually possible here?  That would imply the block has no live predecessors, which shouldn't be possible for a block in a region.  Maybe better to write a generic implementation in case someone tries to copy it elsewhere, though.

This is quadratic in the number of predecessors, for multiple reasons: calling removeIncomingValue with a BasicBlock* implies a scan over all the predecessors, and then actually removing the value requires another linear scan to fill the gap. But not sure if that's likely to matter.


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

https://reviews.llvm.org/D157621