[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:
> 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139872