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

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

In D157621#4587301 <https://reviews.llvm.org/D157621#4587301>, @efriedma wrote:

> 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.

Interesting idea, sounds plausible to me. We probably don't have all that many transforms that can orphan blocks and don't clean them up as a matter of course.



================
Comment at: llvm/lib/Transforms/Instrumentation/ControlHeightReduction.cpp:1786
+      SmallVector<BasicBlock *> DeadPreds;
+      for (PHINode &PN : NewBB->phis()) {
+        for (BasicBlock *Pred : PN.blocks())
----------------
efriedma wrote:
> 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.
The iterator invalidation I'm concerned about here is the case where multiple predecessors need to be removed. I don't think it's valid to iterate PHINode::blocks() while also removing incoming blocks.

We do have generic implementations of the general concept -- what you'd usually do is iterate over predecessors() and call removePredecessor(). The problem is that at this point the CFG is in a somewhat inconsistent state, with the new block cloned but edges not rewritten yet, so predecessors() will not do the right thing and removePredecessor() will assert.

The removal is indeed quadratic, but I don't think it really matters in this case. I think with the APIs we have the only way to make it linear would be to create new PHINodes instead of changing existing ones.




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

https://reviews.llvm.org/D157621