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

[Nikita Popov via Phabricator via llvm-commits]

nikic 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())
----------------
efriedma wrote:
> 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.
I looked around a bit to see whether there are other places that need to remove multiple predecessors from a phi and found quite a few candidates:

https://github.com/llvm/llvm-project/blob/c09d3c1ead632e39c19a78049f6e663b6dbebc4e/llvm/lib/Transforms/Utils/BasicBlockUtils.cpp#L1141-L1147
https://github.com/llvm/llvm-project/blob/ca2eabdb52fa95a001c18e2dd6a15018585e7429/llvm/lib/Transforms/Utils/LoopUtils.cpp#L556-L564
https://github.com/llvm/llvm-project/blob/ca2eabdb52fa95a001c18e2dd6a15018585e7429/llvm/lib/Transforms/Utils/LowerSwitch.cpp#L146-L149
https://github.com/llvm/llvm-project/blob/c09d3c1ead632e39c19a78049f6e663b6dbebc4e/llvm/lib/Transforms/Utils/CloneFunction.cpp#L756-L766
https://github.com/llvm/llvm-project/blob/ca2eabdb52fa95a001c18e2dd6a15018585e7429/llvm/lib/Transforms/Utils/SimplifyCFG.cpp#L5386-L5400
https://github.com/llvm/llvm-project/blob/ca2eabdb52fa95a001c18e2dd6a15018585e7429/llvm/lib/Transforms/Utils/SimplifyCFG.cpp#L5893

So I think adding such an API would make sense. These also show a pattern to avoid the iterator invalidation, which is to do the removal while iterating backwards.

If it's fine with you, I'd prefer to land this change first and introduce an API later, to make this easier to backport.


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

https://reviews.llvm.org/D157621