[PATCH] D142924: [llvm][IfConversion] update successor list when merging INLINEASM_BR
Nick Desaulniers via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Feb 2 14:59:50 PST 2023
nickdesaulniers marked an inline comment as done.
nickdesaulniers added inline comments.
================
Comment at: llvm/lib/CodeGen/IfConversion.cpp:2254
+ if (MO.isMBB() && !ToBBI.BB->isSuccessor(MO.getMBB()))
+ ToBBI.BB->addSuccessor(MO.getMBB(), BranchProbability::getZero());
+
----------------
nickdesaulniers wrote:
> nickdesaulniers wrote:
> > nickdesaulniers wrote:
> > > void wrote:
> > > > void wrote:
> > > > > nickdesaulniers wrote:
> > > > > > nickdesaulniers wrote:
> > > > > > > Perhaps it might be best to see if the prior branch had a branch probability, and if so, reuse that?
> > > > > > >
> > > > > > >
> > > > > > > See calls to `getEdgeProbability` below.
> > > > > > eh maybe not. Pretty sure we only create these edges with 0% probability. The fallthough BB is not an explicit operand.
> > > > > +1. I think we mark all indirect edges as "cold" by default.
> > > > Do we already move the indirect blocks to the end of the function? Or would that be too pessimistic?
> > > I imagine that `Branch Probability Basic Block Placement (block-placement)` would, or `ARM block placement (arm-block-placement)`. But there's no such potential optimization at least for this test case.
> > >
> > > ---
> > >
> > > That said, tailduplication seems broken on this test case...we have 2 unconditional branches to return instructions that don't get replaced with just return instructions...
> > Probably the fault of the `if (PredBB->succ_size() > 1)` guard in `TailDuplicator::canTailDuplicate`
> ```
> diff --git a/llvm/lib/CodeGen/TailDuplicator.cpp b/llvm/lib/CodeGen/TailDuplicator.cpp
> index 865add28f781..5cc70979a9d3 100644
> --- a/llvm/lib/CodeGen/TailDuplicator.cpp
> +++ b/llvm/lib/CodeGen/TailDuplicator.cpp
> @@ -791,7 +791,7 @@ bool TailDuplicator::duplicateSimpleBB(
> bool TailDuplicator::canTailDuplicate(MachineBasicBlock *TailBB,
> MachineBasicBlock *PredBB) {
> // EH edges are ignored by analyzeBranch.
> - if (PredBB->succ_size() > 1)
> + if (PredBB->succ_size() > 1 && !PredBB->mayHaveInlineAsmBr())
> return false;
>
> MachineBasicBlock *PredTBB = nullptr, *PredFBB = nullptr;
> @@ -896,7 +896,7 @@ bool TailDuplicator::tailDuplicate(bool IsSimple, MachineBasicBlock *TailBB,
>
> // Update the CFG.
> PredBB->removeSuccessor(PredBB->succ_begin());
> - assert(PredBB->succ_empty() &&
> + assert((PredBB->succ_empty() || PredBB->mayHaveInlineAsmBr()) &&
> "TailDuplicate called on block with multiple successors!");
> for (MachineBasicBlock *Succ : TailBB->successors())
> PredBB->addSuccessor(Succ, MBPI->getEdgeProbability(TailBB, Succ));
> ```
> produces what I want (from the test case in this patch, though this whole thread is orthogonal to this patch).
https://reviews.llvm.org/D143219 for that. Not sure I ever answered your original question though...
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D142924/new/
https://reviews.llvm.org/D142924
More information about the llvm-commits
mailing list