[PATCH] D84264: [SCCP] Directly remove non-feasible edges

[Nikita Popov via Phabricator via llvm-commits]

nikic marked 2 inline comments as done.
nikic added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/SCCP.cpp:1834
+            isa<IndirectBrInst>(TI)) &&
+           "Terminator must be a br, switch or indirectbr");
+    BasicBlock *SingleSucc = *FeasibleSuccessors.begin();
----------------
efriedma wrote:
> Can we skip creating the unconditional branch if the terminator is already an unconditional branch?
> 
> Is it possible for this code to see other branches which inherently only have one successor, e.g. catchret?
Any of this code is only executed if there is at least one non-feasible successor, i.e. only cases for which SCCP supports feasible edge analysis (that is more precise than "all successors"). Those are conditional branches, switches and indirectbr, thus the types in the assert.


================
Comment at: llvm/lib/Transforms/Scalar/SCCP.cpp:1836
+    BasicBlock *SingleSucc = *FeasibleSuccessors.begin();
+    BranchInst::Create(SingleSucc, BB);
+    TI->eraseFromParent();
----------------
efriedma wrote:
> nikic wrote:
> > efriedma wrote:
> > > Even if only one basic block is a possible successor, there could still be multiple edges, for example in a switch.  (This matters because you might need to update PHI nodes.)
> > > 
> > > ConstantFoldTerminator does some stuff with profile metadata; do we need something similar here?
> > The profile metadata adjustment is only done when merging cases into the default destination. We don't need to adjust metadata if we convert into an unconditional branch.
> > Even if only one basic block is a possible successor, there could still be multiple edges, for example in a switch. (This matters because you might need to update PHI nodes.)
> 
> Any response to this?
Sorry, I misunderstood what you meant here and thought this was resolved by the previous change. I've now added two additional test cases in https://github.com/llvm/llvm-project/commit/e20b3079c14d8e68ebc90125eb95156f2e1ee9e4 and fixed this to remove all but one of the edges.


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

https://reviews.llvm.org/D84264