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

[James Y Knight via Phabricator via llvm-commits]

jyknight added inline comments.


================
Comment at: llvm/lib/CodeGen/CallBrPrepare.cpp:75
     auto *CBR = dyn_cast<CallBrInst>(BB.getTerminator());
-    if (!CBR)
-      continue;
-    // TODO: something interesting.
-    // https://discourse.llvm.org/t/rfc-syncing-asm-goto-with-outputs-with-gcc/65453/8
+    if (ShouldRun(CBR))
+      CBRs.push_back(CBR);
----------------
Doesn't seem to add anything to have "ShouldRun" as a separate function. I'd just inline the check here.

If not that, rename it; the name "ShouldRun" is not adding any understandability here.


================
Comment at: llvm/lib/CodeGen/CallBrPrepare.cpp:92-93
+    // Do one pass to find the indices of unique indirect successors BEFORE
+    // attempting to split critical edges (otherwise the BasicBlock inserted
+    // in the Visited Set below will not catch duplicates; it would get
+    // updated to the new synthetic block while iterating this loop). Start
----------------
I don't understand this comment. With a single loop, if we iterate over the successors in order, and successors 1 and 3 are both to the same block, the `MergeIdenticalEdges` option should ensure that when we call `SplitKnownCriticalEdge` on 1, it'll also rewrite 3. 

Oh, I see -- I think you needed `isCriticalEdge(CBR, i, /*AllowIdenticalEdges=*/true)`, so that we don't report the edge as critical when it's only used from a single block (and won't split the already-split edge!)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139872