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

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers added inline comments.


================
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
----------------
jyknight wrote:
> 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!)
> 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.

Correct, see `@split_me1` in llvm/test/CodeGen/AArch64/callbr-prepare.ll which covers this part of the code.

> 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!)

I don't think setting `AllowIdenticalEdges` to `true` works.  I have vague memories of trying to use it, but I can't recall specifically why I wasn't able to use it and haven't been able to rework this block via setting `AllowIdenticalEdges` to `true`.

The comment also mentions `SplitCriticalEdge` rather than `SplitKnownCriticalEdge`. That may be an artifact from an earlier revision.  At the least, I should fix that part of the comment.

Do you have thoughts on how I could better rephrase the comment block you didn't understand (assuming now that you do)?

Are you asking that I try to use `AllowIdenticalEdges` set to `true`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139872