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

llvm/lib/CodeGen/IfConversion.cpp

 /// to the end of the function.  If AddEdges is true, i.e., when FromBBI's
 /// branch is being moved, add those successor edges to ToBBI and remove the old
 /// edge from ToBBI to FromBBI.
 void IfConverter::MergeBlocks(BBInfo &ToBBI, BBInfo &FromBBI, bool AddEdges) {
   MachineBasicBlock &FromMBB = *FromBBI.BB;
   assert(!FromMBB.hasAddressTaken() &&
          "Removing a BB whose address is taken!");
 
+  // If we're about to splice an INLINEASM_BR from FromBBI, we need to update
+  // ToBBI's successor list accordingly.
+  if (FromMBB.mayHaveInlineAsmBr())
+    for (MachineInstr &MI : FromMBB)
+      if (MI.getOpcode() == TargetOpcode::INLINEASM_BR)
+        for (MachineOperand &MO : MI.operands())
+          if (MO.isMBB() && !ToBBI.BB->isSuccessor(MO.getMBB()))
+            ToBBI.BB->addSuccessor(MO.getMBB(), BranchProbability::getZero());
+
   // In case FromMBB contains terminators (e.g. return instruction),
   // first move the non-terminator instructions, then the terminators.
   MachineBasicBlock::iterator FromTI = FromMBB.getFirstTerminator();
   MachineBasicBlock::iterator ToTI = ToBBI.BB->getFirstTerminator();
   ToBBI.BB->splice(ToTI, &FromMBB, FromMBB.begin(), FromTI);
 
   // If FromBB has non-predicated terminator we should copy it at the end.
   if (FromTI != FromMBB.end() && !TII->isPredicated(*FromTI))

llvm/test/CodeGen/ARM/inlineasmbr-if-cvt.mir

 stack:           []
 callSites:       []
 debugValueSubstitutions: []
 constants:       []
 machineFunctionInfo: {}
 body:             |
   ; CHECK-LABEL: name: tusb1210_chg_det_work
   ; CHECK: bb.0.entry:
-  ; CHECK-NEXT:   successors:
+  ; CHECK-NEXT:   successors: %bb.1(0x80000000), %bb.2(0x80000000)
   ; CHECK-NEXT:   liveins: $r0
   ; CHECK-NEXT: {{  $}}
   ; CHECK-NEXT:   TSTri killed renamable $r0, 1, 14 /* CC::al */, $noreg, implicit-def $cpsr
   ; CHECK-NEXT:   renamable $r0 = MOVi 0, 1 /* CC::ne */, $cpsr, $noreg
-  ; CHECK-NEXT:   dead renamable $r0 = MOVi 1, 0 /* CC::eq */, killed $cpsr, $noreg, implicit killed $r0
-  ; FIXME: %bb.-1 is a dangling reference!!!
-  ; CHECK-NEXT:   INLINEASM_BR &".word b, ${1:l}, ${0:c}\0A\09", 9 /* sideeffect mayload attdialect */, 13 /* imm */, 0, 13 /* imm */, %bb.-1
+  ; CHECK-NEXT:   renamable $r0 = MOVi 1, 0 /* CC::eq */, killed $cpsr, $noreg, implicit killed $r0
+  ; CHECK-NEXT:   INLINEASM_BR &".word b, ${1:l}, ${0:c}\0A\09", 9 /* sideeffect mayload attdialect */, 13 /* imm */, 0, 13 /* imm */, %bb.1
+  ; CHECK-NEXT:   B %bb.2
+  ; CHECK-NEXT: {{  $}}
+  ; CHECK-NEXT: bb.1.if.end.sink.split (machine-block-address-taken, inlineasm-br-indirect-target):
+  ; CHECK-NEXT:   successors: %bb.2(0x80000000)
+  ; CHECK-NEXT:   liveins: $r0
+  ; CHECK-NEXT: {{  $}}
+  ; CHECK-NEXT:   renamable $r1 = MOVi 0, 14 /* CC::al */, $noreg, $noreg
+  ; CHECK-NEXT:   STRi12 killed renamable $r0, killed renamable $r1, 0, 14 /* CC::al */, $noreg :: (store (s32) into `ptr null`)
+  ; CHECK-NEXT: {{  $}}
+  ; CHECK-NEXT: bb.2.if.end:
   ; CHECK-NEXT:   BX_RET 14 /* CC::al */, $noreg
   bb.0.entry:
     successors: %bb.4(0x40000000), %bb.1(0x40000000)
     liveins: $r0
 
     TSTri killed renamable $r0, 1, 14 /* CC::al */, $noreg, implicit-def $cpsr
     Bcc %bb.4, 1 /* CC::ne */, killed $cpsr