Skip to content

[SelectionDAGBuilder] Look for appropriate INLINEASM_BR instruction to verify #152591

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Aug 12, 2025

Conversation

XChy
Copy link
Member

@XChy XChy commented Aug 7, 2025

Partially fix #149023.
The original code MRI.def_begin(Reg)->getParent() may return the incorrect MI, as the physical register Reg may have multiple definitions.
This patch selects the correct MI to verify by comparing the MBB of each definition.

New testcase hangs with -O1/2/3 enabled. The BranchFolding may be to blame.

@XChy XChy requested review from nikic, RKSimon, dtcxzyw and topperc August 7, 2025 20:30
@XChy XChy added the llvm:SelectionDAG SelectionDAGISel as well label Aug 7, 2025
@llvmbot
Copy link
Member

llvmbot commented Aug 7, 2025

@llvm/pr-subscribers-backend-x86

@llvm/pr-subscribers-llvm-selectiondag

Author: XChy (XChy)

Changes

Partially fix #149023.
The original code MRI.def_begin(Reg)->getParent() may return the incorrect MI, as the physical register Reg may have multiple definitions.
This patch selects the correct MI to verify by comparing the MBB of each definition.

New testcase hangs with -O1/2/3 enabled. The BranchFolding may be to blame.


Full diff: https://github.com/llvm/llvm-project/pull/152591.diff

2 Files Affected:

  • (modified) llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp (+18-3)
  • (added) llvm/test/CodeGen/X86/callbr-asm-loop.ll (+50)
diff --git a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
index d0815e9f51822..c345c6207dfee 100644
--- a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
@@ -12731,23 +12731,37 @@ void SelectionDAGBuilder::visitVectorSplice(const CallInst &I) {
 // increase in virtreg number from there. If a callbr has no outputs, then it
 // should not have a corresponding callbr landingpad; in fact, the callbr
 // landingpad would not even be able to refer to such a callbr.
-static Register FollowCopyChain(MachineRegisterInfo &MRI, Register Reg) {
+static Register FollowCopyChain(MachineRegisterInfo &MRI, Register Reg,
+                                MachineBasicBlock *CallBrMBB) {
   MachineInstr *MI = MRI.def_begin(Reg)->getParent();
   // There is definitely at least one copy.
   assert(MI->getOpcode() == TargetOpcode::COPY &&
          "start of copy chain MUST be COPY");
   Reg = MI->getOperand(1).getReg();
+
+  // If the copied register in the first copy must be virtual.
+  assert(Reg.isVirtual() && "expected COPY of virtual register");
   MI = MRI.def_begin(Reg)->getParent();
+
   // There may be an optional second copy.
   if (MI->getOpcode() == TargetOpcode::COPY) {
     assert(Reg.isVirtual() && "expected COPY of virtual register");
     Reg = MI->getOperand(1).getReg();
     assert(Reg.isPhysical() && "expected COPY of physical register");
-    MI = MRI.def_begin(Reg)->getParent();
+
+    // Look for the machine callbr instruction.
+    for (auto Def : MRI.def_operands(Reg))
+      if (Def.getParent()->getOpcode() == TargetOpcode::INLINEASM_BR &&
+          Def.getParent()->getParent() == CallBrMBB) {
+        MI = Def.getParent();
+        break;
+      }
   }
+
   // The start of the chain must be an INLINEASM_BR.
   assert(MI->getOpcode() == TargetOpcode::INLINEASM_BR &&
          "end of copy chain MUST be INLINEASM_BR");
+
   return Reg;
 }
 
@@ -12766,6 +12780,7 @@ void SelectionDAGBuilder::visitCallBrLandingPad(const CallInst &I) {
   MachineRegisterInfo &MRI = DAG.getMachineFunction().getRegInfo();
 
   Register InitialDef = FuncInfo.ValueMap[CBR];
+  MachineBasicBlock *CallBrMBB = FuncInfo.getMBB(CBR->getParent());
   SDValue Chain = DAG.getRoot();
 
   // Re-parse the asm constraints string.
@@ -12789,7 +12804,7 @@ void SelectionDAGBuilder::visitCallBrLandingPad(const CallInst &I) {
       // getRegistersForValue may produce 1 to many registers based on whether
       // the OpInfo.ConstraintVT is legal on the target or not.
       for (Register &Reg : OpInfo.AssignedRegs.Regs) {
-        Register OriginalDef = FollowCopyChain(MRI, InitialDef++);
+        Register OriginalDef = FollowCopyChain(MRI, InitialDef++, CallBrMBB);
         if (OriginalDef.isPhysical())
           FuncInfo.MBB->addLiveIn(OriginalDef);
         // Update the assigned registers to use the original defs.
diff --git a/llvm/test/CodeGen/X86/callbr-asm-loop.ll b/llvm/test/CodeGen/X86/callbr-asm-loop.ll
new file mode 100644
index 0000000000000..f2a63db9107f2
--- /dev/null
+++ b/llvm/test/CodeGen/X86/callbr-asm-loop.ll
@@ -0,0 +1,50 @@
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py UTC_ARGS: --version 5
+
+; RUN: llc -O0 -mtriple=i686-- -verify-machineinstrs < %s | FileCheck %s
+
+; Test that causes multiple defs of %eax.
+; FIXME: The testcase hangs with -O1/2/3 enabled.
+define i32 @loop1() {
+; CHECK-LABEL: loop1:
+; CHECK:       # %bb.0: # %entry
+; CHECK-NEXT:    pushl %esi
+; CHECK-NEXT:    .cfi_def_cfa_offset 8
+; CHECK-NEXT:    .cfi_offset %esi, -8
+; CHECK-NEXT:    jmp .LBB0_1
+; CHECK-NEXT:  .LBB0_1: # %tailrecurse
+; CHECK-NEXT:    # =>This Inner Loop Header: Depth=1
+; CHECK-NEXT:    xorl %eax, %eax
+; CHECK-NEXT:    movl $1, %edx
+; CHECK-NEXT:    #APP
+; CHECK-NEXT:    #NO_APP
+; CHECK-NEXT:    movl %eax, %ecx
+; CHECK-NEXT:    movl %edx, %esi
+; CHECK-NEXT:    jmp .LBB0_3
+; CHECK-NEXT:  .LBB0_2: # Inline asm indirect target
+; CHECK-NEXT:    # %tailrecurse.tailrecurse.backedge_crit_edge
+; CHECK-NEXT:    # in Loop: Header=BB0_1 Depth=1
+; CHECK-NEXT:    # Label of block must be emitted
+; CHECK-NEXT:  .LBB0_3: # %tailrecurse.backedge
+; CHECK-NEXT:    # in Loop: Header=BB0_1 Depth=1
+; CHECK-NEXT:    jmp .LBB0_1
+; CHECK-NEXT:  .LBB0_4: # Inline asm indirect target
+; CHECK-NEXT:    # %lab2.split
+; CHECK-NEXT:    # Label of block must be emitted
+; CHECK-NEXT:    movl %edx, %eax
+; CHECK-NEXT:    popl %esi
+; CHECK-NEXT:    .cfi_def_cfa_offset 4
+; CHECK-NEXT:    retl
+entry:
+  br label %tailrecurse
+
+tailrecurse:
+  %0 = callbr { i32, i32 } asm "", "={ax},={dx},0,1,!i,!i"(i32 0, i32 1) #1
+          to label %tailrecurse.backedge [label %tailrecurse.backedge, label %lab2.split]
+
+tailrecurse.backedge:
+  br label %tailrecurse
+
+lab2.split:
+  %asmresult5 = extractvalue { i32, i32 } %0, 1
+  ret i32 %asmresult5
+}

@arsenm arsenm changed the title [SelectionDAGBuilder] Look for apropriate INLINEASM_BR instruction to verify [SelectionDAGBuilder] Look for approriate INLINEASM_BR instruction to verify Aug 8, 2025
Comment on lines 12752 to 12758
// Look for the machine callbr instruction.
for (auto Def : MRI.def_operands(Reg))
if (Def.getParent()->getOpcode() == TargetOpcode::INLINEASM_BR &&
Def.getParent()->getParent() == CallBrMBB) {
MI = Def.getParent();
break;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you really do this in a SelectionDAGBuilder::visit call? This is depending on seeing blocks that were already selected in an incomplete function, later blocks will not exist yet. I would expect you would need to do some other kind of processing in FunctionLoweringInfo

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tracking the relationship between the output register and the callbr instruction still requires seeing the selected block of the callbr instruction. We may be able to scan the block in FunctionLoweringInfo, but it is not worthwhile to me. Maybe we can drop the verification for the second case in the comment. What's your opinion? @arsenm

Copy link
Contributor

@arsenm arsenm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think what FollowCopyChain is doing is dubious, but as long as it's only looking at virtual registers it's less likely to break anything

@arsenm arsenm changed the title [SelectionDAGBuilder] Look for approriate INLINEASM_BR instruction to verify [SelectionDAGBuilder] Look for appropriate INLINEASM_BR instruction to verify Aug 12, 2025
@arsenm arsenm enabled auto-merge (squash) August 12, 2025 12:32
@arsenm arsenm merged commit 2a49719 into llvm:main Aug 12, 2025
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend:X86 llvm:SelectionDAG SelectionDAGISel as well
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clang assertion failure at O2 "Assertion `MI->getOpcode() == TargetOpcode::INLINEASM_BR && "end of copy chain MUST be INLINEASM_BR"' failed."
3 participants