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

[Nikita Popov via Phabricator via llvm-commits]

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


================
Comment at: llvm/lib/Transforms/Scalar/SCCP.cpp:1826
+    Updates.push_back({DominatorTree::Delete, BB, Succ});
+    Succ->removePredecessor(BB);
+  }
----------------
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.


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


================
Comment at: llvm/lib/Transforms/Scalar/SCCP.cpp:1839
+  } else {
+    llvm_unreachable("Either all successors are feasible, or exactly one is");
   }
----------------
efriedma wrote:
> Is it possible that no successor is feasible?
This shouldn't be possible right now, as we always force one successor to be feasible, even if it was undef/unknown. It might be possible in the future if we want to make use of branch-on-undef being UB and all successors being non-feasible.


================
Comment at: llvm/test/Transforms/SCCP/switch-constantfold-crash.ll:8
 ; CHECK-NEXT:  bb:
-; CHECK-NEXT:    br label %bb9
+; CHECK-NEXT:    br label [[BB9:%.*]]
 ; CHECK:       bb6:
----------------
fhahn wrote:
> These checks seem odd. We care basically just creating a FileCheck variable, but never match the variable elsewhere, right? So we aren't actually checking if we branch to `bb9` here, unless I am missing something.
Yes, this is a weakness of update_test_checks generated check lines. Do you want me to manually undo these?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84264