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

[Nikita Popov via Phabricator via llvm-commits]

nikic marked 5 inline comments as done.
nikic 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});
----------------
efriedma wrote:
> Iterating over a smallptrset is non-deterministic.
The updated version no longer iterates over SmallPtrSet :)


================
Comment at: llvm/lib/Transforms/Scalar/SCCP.cpp:1826
+    Updates.push_back({DominatorTree::Delete, BB, Succ});
+    Succ->removePredecessor(BB);
+  }
----------------
nikic wrote:
> efriedma wrote:
> > I'm not sure calling removePredecessor once is enough if there are multiple edges to the same successor, for example in a switch.
> I think you are right. removePredecessor() is basically removeIncomingValue() on all phi nodes, and that function only removes a single incoming value. I'll try to come up with a failing test case.
This is now fixed. Previously the test case from https://github.com/llvm/llvm-project/commit/eae6bb3807977a2998ac9114a1d6ecb6bdafc3cd crashed.


================
Comment at: llvm/test/Transforms/SCCP/switch.ll:21
 
 define i32 @test_duplicate_successors_phi(i1 %c, i32 %x) {
 ; CHECK-LABEL: @test_duplicate_successors_phi(
----------------
This test change shows another weakness of the previous approach: Even though we already determined that here only the default edge is feasible, we did not make use of this fact, because the target block is still reachable through a different edge. Because ConstantFoldTerminator was called for the predecessors of unreachable blocks, this case was missed.


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

https://reviews.llvm.org/D84264