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

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/SCCP.cpp:1824
+  SmallVector<DominatorTree::UpdateType, 8> Updates;
+  for (BasicBlock *Succ : NonFeasibleSuccessors) {
+    Updates.push_back({DominatorTree::Delete, BB, Succ});
----------------
Iterating over a smallptrset is non-deterministic.


================
Comment at: llvm/lib/Transforms/Scalar/SCCP.cpp:1826
+    Updates.push_back({DominatorTree::Delete, BB, Succ});
+    Succ->removePredecessor(BB);
+  }
----------------
I'm not sure calling removePredecessor once is enough if there are multiple edges to the same successor, for example in a switch.


================
Comment at: llvm/lib/Transforms/Scalar/SCCP.cpp:1836
+    BasicBlock *SingleSucc = *FeasibleSuccessors.begin();
+    BranchInst::Create(SingleSucc, BB);
+    TI->eraseFromParent();
----------------
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?


================
Comment at: llvm/lib/Transforms/Scalar/SCCP.cpp:1839
+  } else {
+    llvm_unreachable("Either all successors are feasible, or exactly one is");
   }
----------------
Is it possible that no successor is feasible?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84264