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

[Eli Friedman via Phabricator via llvm-commits]

efriedma accepted this revision.
efriedma added a comment.
This revision is now accepted and ready to land.

LGTM



================
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:
> > 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.
Sure, leaving the new API for a followup sounds fine.


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

https://reviews.llvm.org/D157621