[llvm][IfConversion] update successor list when merging INLINEASM_BR

[nickdesaulniers]

Summary

If this successor list is not correct, then branch-folding may
incorrectly think that the indirect target is dead and remove it. This
results in a dangling reference to the removed block as an operand to
the INLINEASM_BR, which later will get AsmPrinted into code that doesn't
assemble.

This was made more obvious by, but is not a regression of
https://reviews.llvm.org/D130316.

Fixes: https://github.com/llvm/llvm-project/issues/60346

[nickdesaulniers]

• update commit message

[nickdesaulniers]

llvm/lib/CodeGen/IfConversion.cpp

2254

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.

[nickdesaulniers]

llvm/lib/CodeGen/IfConversion.cpp

2254

eh maybe not. Pretty sure we only create these edges with 0% probability. The fallthough BB is not an explicit operand.

[nickdesaulniers]

I suspect that https://reviews.llvm.org/D79794 might have been the culprit here.

[void]

llvm/lib/CodeGen/IfConversion.cpp

2254

+1. I think we mark all indirect edges as "cold" by default.

[void]

llvm/lib/CodeGen/IfConversion.cpp

2254

Do we already move the indirect blocks to the end of the function? Or would that be too pessimistic?

[nickdesaulniers]

llvm/lib/CodeGen/IfConversion.cpp

2254

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

[nickdesaulniers]

llvm/lib/CodeGen/IfConversion.cpp

2254

Probably the fault of the if (PredBB->succ_size() > 1) guard in TailDuplicator::canTailDuplicate

[nickdesaulniers]

llvm/lib/CodeGen/IfConversion.cpp

2254

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).

[nickdesaulniers]

llvm/lib/CodeGen/IfConversion.cpp

2254

https://reviews.llvm.org/D143219 for that. Not sure I ever answered your original question though...

[nickdesaulniers]

(I'm going to add a machine verifier test for this first)

[efriedma]

LGTM

[nickdesaulniers]

In D142924#4107194, @nickdesaulniers wrote:

(I'm going to add a machine verifier test for this first)

Huh, the checks I added in https://reviews.llvm.org/D130290 catch this. How come machine verifier doesn't run from clang? Is there a way to force it on from clang?

Looks like -mllvm -verify-machineinstrs does it.

I'll add -verify-machineinstrs to this test at least.

[nickdesaulniers]

• add -verify-machineinstrs now that this test passes with it added

[nickdesaulniers]

In D142924#4110734, @nickdesaulniers wrote:

In D142924#4107194, @nickdesaulniers wrote:

(I'm going to add a machine verifier test for this first)

Huh, the checks I added in https://reviews.llvm.org/D130290 catch this. How come machine verifier doesn't run from clang? Is there a way to force it on from clang?

Looks like -mllvm -verify-machineinstrs does it.

Also -DLLVM_ENABLE_EXPENSIVE_CHECKS=ON seems to enable -verify-machineinstrs by default based on a build report I just got that I think just tested the parent commit (a known failure with -verify-machineinstrs).