[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
----------------
nickdesaulniers wrote:
> nickdesaulniers wrote:
> > 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`?
> For instance, I would have like for this code to simply have been something like:
> ```
> for (BasicBlock *BB : CBR->getIndirectDests())
>   if (BasicBlock *Synth = SplitCriticalEdge(CBR, GetSuccessorNumber(CBR->getParent(), BB), Options))
>     Changed = true;
> ```
> but that doesn't work, hence the two loops and (obtuse) commentary.
So I think the issue here is what happens if the direct and indirect destination are the same.  Maybe ` @split_me5` test case added later, where we don't want to split both edges, vs two indirect branches that are the same, in case we'd like to split both of them  `@split_me1` but only once.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139872