[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:
> > 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.
@jyknight what are your thoughts on rewording the comment block as such (sorry, the block has shifted around from edits since your comment; we're referring to the comment blocks in `CallBrPrepare::SplitCriticalEdges`)

```
Conceptually, these two loops are doing:

for (BasicBlock *Dest : CBR->getIndirectDests())
  if (BasicBlock *Synth = SplitCriticalEdge(CBR, GetSuccessorNumber(CBR->getParent(), Dest)))
    Changed = true;

But there is a problem resulting in two special cases; a single callbr instruction may have multiple edges to the
same destination.  Consider the first case:

  %0 = callbr ... to label %x [label %y, label %y]

We want to split the edge to %y once, not twice so that both instance of %y go to the new destination.
Now consider the second case:

  %0 = callbr ... to label %x [label %x]

We want to split the indirect edge, but not the direct edge (since the direct edge doesn't
have the problem of where to store copies).  This pair of sequential loops is necessary to
handle what is logically a pass over indirect edges to split them.
```
(with necessary word wrapping, since I haven't typed it out in code yet)  (I would then delete all existing comments in that method).

Thoughts?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139872