[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:96
+    // at index 1 to avoid the default destination.
+    for (unsigned i = 1, e = CBR->getNumSuccessors(); i != e; ++i) {
+      // The same destination may appear multiple times in a callbr
----------------
nickdesaulniers wrote:
> void wrote:
> > nickdesaulniers wrote:
> > > void wrote:
> > > > You can iterate over the indirect destinations. Something like:
> > > > 
> > > > ```
> > > > for (BasicBlock *BB : CBR->getIndirectDests()) {
> > > > }
> > > > ```
> > > I would have preferred to use range-for iteration here. IIRC, the issue with that approach was that `isCriticalEdge` takes an `unsigned` as the successor number (rather than the `BasicBlock` successor itself.  If it did, that would probably have improved the ergonomics here.  Let me know if you think I should add an overload of `isCriticalEdge` perhaps, or if I missed something else?
> > `isCriticalEdge` can also take a BB:
> > 
> > ```
> > llvm/include/llvm/Analysis/CFG.h:
> > bool isCriticalEdge(const Instruction *TI, const BasicBlock *Succ,
> >                     bool AllowIdenticalEdges = false);
> > ```
> Ah, it was `SplitKnownCriticalEdge` used later outside of this loop that expected the index of the successor number.  That's why this loop builds up a vector of indices; the next loop will then split them.  So I don't think I can change this first loop, but let me know if I'm still missing something.
It looks like `llvm::GetSuccessorNumber` could be used. So even if `SplitKnownCriticalEdge` needs an index, I could use range-for then `llvm::GetSuccessorNumber`.  That may be more readable, but it looks like it would be O(N^2) rather than two sequential O(N) loops? I guess N is likely too small to matter though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139872