[PATCH] D139872: [llvm][CallBrPrepare] split critical edges

[Arthur Eubanks via Phabricator via llvm-commits]

aeubanks added inline comments.


================
Comment at: llvm/lib/CodeGen/CallBrPrepare.cpp:59
 char CallBrPrepare::ID = 0;
 INITIALIZE_PASS(CallBrPrepare, DEBUG_TYPE, "Prepare callbr", false, false)
 
----------------
this needs to be changed to be like [this](https://github.com/llvm/llvm-project/blob/1b753240d50a930c12c42e2f230804db3dccde96/llvm/lib/Analysis/DominanceFrontier.cpp#L32) to initialize dom tree (annoying legacy-PM boilerplate)


================
Comment at: llvm/lib/CodeGen/CallBrPrepare.cpp:67
 
-bool CallBrPrepare::runOnFunction(Function &Fn) {
+bool CallBrPrepare::ShouldRun(const CallBrInst *CBR) const {
+  return CBR && !CBR->getType()->isVoidTy() && !CBR->use_empty();
----------------
both `ShouldRun` can be static


================
Comment at: llvm/lib/CodeGen/CallBrPrepare.cpp:71
+
+bool CallBrPrepare::ShouldRun(Function &Fn,
+                              SmallVectorImpl<CallBrInst *> &CBRs) const {
----------------
I'd just return the list of CallBrInsts and check if it's empty in the caller, that's a lot more explicit


================
Comment at: llvm/lib/CodeGen/CallBrPrepare.cpp:90-91
+  for (CallBrInst *CBR : CBRs) {
+    Visited.clear();
+    UniqueCriticalSuccessorIndices.clear();
+
----------------
moving these variables into the loop makes this less errorprone


================
Comment at: llvm/lib/CodeGen/CallBrPrepare.cpp:114
+      assert(Synth && "Failed to split a known critical edge from callbr");
+      if (Synth)
+        Changed = true;
----------------
unnecessary if


================
Comment at: llvm/lib/CodeGen/CallBrPrepare.cpp:128-129
+
+  auto *DTWP = getAnalysisIfAvailable<DominatorTreeWrapperPass>();
+  DominatorTree *DT = DTWP ? &DTWP->getDomTree() : nullptr;
+
----------------
nickdesaulniers wrote:
> @aeubanks does this look correct?
yup, lg


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139872