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

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: llvm/lib/Transforms/Instrumentation/ControlHeightReduction.cpp:1786
+      SmallVector<BasicBlock *> DeadPreds;
+      for (PHINode &PN : NewBB->phis()) {
+        for (BasicBlock *Pred : PN.blocks())
----------------
nikic wrote:
> 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.
> 
> 
> I don't think it's valid to iterate PHINode::blocks() while also removing incoming blocks.

Oh, sure.  I got confused because of the use of make_early_inc_range.

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

It probably makes sense to add an API similar to std::remove_if to PHINode... but I agree it's unlikely to matter.


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

https://reviews.llvm.org/D157621