[globalisel] Select register bank for DBG_VALUE

[LuoYuanke]

Summary

The register operand of DBG_VALUE is not selected to a proper register
bank in both AArch64 and X86. This would cause getRegClass crash after
global ISel. After discussion, we think the MIR should assume all
vritual register should be set proper register class after global ISel,
so this patch is to fix the gap of DBG_VALUE for AArch64 and X86.

[LuoYuanke]

This patch is to fix the global ISel issue encountered in D126771 and http://45.33.8.238/macm1/37938/step_11.txt.

[arsenm]

llvm/lib/CodeGen/GlobalISel/RegBankSelect.cpp

606–608

This should get a mir test that just runs regbankselect

llvm/lib/Target/AArch64/GISel/AArch64InstructionSelector.cpp

910–911

I would expect this to be handled by generic code. However, last I looked at handling selection of another pseudo generically, I think we were missing API to allow generic code to constrain registers

912

This should guard the call, not be in it

920

While getType does check for physical registers, I think it shouldn't. Can you change !Reg to Reg.isPhysical()?

928–929

I don't understand why you are looping over to precheck all operands. You can process each register individually

937–952

This looks unnecessarily complicated, but I do think we have some API issues here

[LuoYuanke]

llvm/lib/CodeGen/GlobalISel/RegBankSelect.cpp

606–608

We have test/CodeGen/AArch64/GlobalISel/arm64-regbankselect.mir to cover this scenario.

llvm/lib/Target/AArch64/GISel/AArch64InstructionSelector.cpp

920

This follow the same check of RegBankSelect.cpp:730. The RegBankSelect accept 0 register, but not accept physical register. I can add another check for Reg.isPhysical().

928–929

The precheck is to make sure there is no physical register or invalid type for an operand. If there is one, then we bail out for the whole operands. This is to avoid such scenario we handled some virtual register before we encounter a physical register operand.
We either hanldle all operands or we abandon for the whole instruction.

[LuoYuanke]

llvm/lib/Target/X86/X86RegisterBankInfo.cpp

117

This scenario can be covered by test/CodeGen/X86/GlobalISel/x86-calllowering-dbg-trunc.ll.

133

This scenario can be covered by test/CodeGen/X86/GlobalISel/x86-calllowering-dbg-trunc.ll.

[LuoYuanke]

llvm/lib/CodeGen/GlobalISel/RegBankSelect.cpp

732

There are 3 cases that I encounter when debuging test case failure.

1. 0 register.
2. physical register.
3. virtual register that has been assigned to a register class.

I changed the code to check Reg.isPhysical(), but I got a failure for the case 3. So I have to check if Ty is valid here.

DBG_VALUE %0:gpr32, $noreg, !"6", !DIExpression(), debug-location !35; test/CodeGen/AArch64/GlobalISel/arm64-regbankselect.mir:6:1 line no:6

[LuoYuanke]

Address Arsenault's comments.

[arsenm]

llvm/lib/CodeGen/GlobalISel/RegBankSelect.cpp

730

Typo virutal

734

I wouldn't assume if one operand doesn't need assignment, then none do. Treat each operand individually

llvm/test/CodeGen/AArch64/GlobalISel/select-hint.mir

17–21

These baseline check updates should be pre-committed

[LuoYuanke]

llvm/lib/CodeGen/GlobalISel/RegBankSelect.cpp

734

Yes, I agree, but this patch just to be conservative to constrain the logic in handling debug instruction, so that it can avoid causing much regression.
I think inline assembly also has such issue, because it may be mixed with physical register and virtual register.
How about adding a TODO here, so that we can improve it in another patch that handle the physical register or register that is assigned register class in underlying code?

[LuoYuanke]

Address Arsenault's comments.

[LuoYuanke]

Add TODO for debug instructions and rebase.

[LuoYuanke]

@arsenm, is there anything more I need to address?

[arsenm]

llvm/lib/CodeGen/GlobalISel/RegBankSelect.cpp

734

I don't see this as conservative, just weirdly inconsistent. It would simplify the code and be more correct to just handle all virtual register operands

llvm/lib/Target/AArch64/GISel/AArch64InstructionSelector.cpp

928–929

Same, better to have simpler code that handles each register individually

[LuoYuanke]

Address Arsenault's comments.

[LuoYuanke]

@arsenm, any other comments?

[LuoYuanke]

ping

[arsenm]

LGTM, although this leaves AMDGPU and all other targets broken. I'm going to file a GitHub issue for this

llvm/lib/CodeGen/RegisterBankInfo.cpp

452–454 ↗

(On Diff #445288)

I almost think debug instrs shouldn't go through applyDefaultMapping but maybe it is easier this way

llvm/lib/Target/AArch64/GISel/AArch64InstructionSelector.cpp

910

This should really go to generic code, but I won't insist on you doing that in this patch. We still need a better generic API for constraining. Can you add a fix to move this?

llvm/lib/Target/X86/X86InstructionSelector.cpp

235–237

Same, should get a fixme

[arsenm]

Filed https://github.com/llvm/llvm-project/issues/57010

[LuoYuanke]

Address Arsenault's comments. Thank you for review.

llvm/lib/CodeGen/GlobalISel/RegBankSelect.cpp

 //==- llvm/CodeGen/GlobalISel/RegBankSelect.cpp - RegBankSelect --*- C++ -*-==//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 /// \file
for (RepairingPlacement &RepairPt : RepairPts) {
 
     switch (RepairPt.getKind()) {
     case RepairingPlacement::Reassign:
       assert(ValMapping.NumBreakDowns == 1 &&
              "Reassignment should only be for simple mapping");
       MRI->setRegBank(Reg, *ValMapping.BreakDown[0].RegBank);
       break;
     case RepairingPlacement::Insert:
+      // Don't insert additional instruction for debug instruction.
+      if (MI.isDebugInstr())
+        break;
       OpdMapper.createVRegs(OpIdx);
       if (!repairReg(MO, ValMapping, RepairPt, OpdMapper.getVRegs(OpIdx)))
         return false;
       break;
     default:
       llvm_unreachable("Other kind should not happen");
     }
   }
while (!WorkList.empty()) {
       if (isTargetSpecificOpcode(MI.getOpcode()) && !MI.isPreISelOpcode())
         continue;
 
       // Ignore inline asm instructions: they should use physical
       // registers/regclasses
       if (MI.isInlineAsm())
         continue;
 
-      // Ignore debug info.
-      if (MI.isDebugInstr())
+      if (MI.isDebugInstr()) {
+        bool Ignore = false;
+        for (MachineOperand &MO : MI.operands()) {
+          if (!MO.isReg())
+            continue;
+          Register Reg = MO.getReg();
+          if (!Reg)
+            continue;
+          // If there is physical register or virtual register that has been
+          // assigned to a register class, ignore debug info.
+          LLT Ty = MRI->getType(Reg);
+          if (!Ty.isValid()) {
+            Ignore = true;
+            break;
+          }
+        }
+        if (Ignore)
           continue;
+      }
 
       // Ignore IMPLICIT_DEF which must have a regclass.
       if (MI.isImplicitDef())
         continue;
 
       if (!assignInstr(MI)) {
         reportGISelFailure(MF, *TPC, *MORE, "gisel-regbankselect",
                            "unable to map instruction", MI);

llvm/lib/Target/AArch64/GISel/AArch64InstructionSelector.cpp

 //===- AArch64InstructionSelector.cpp ----------------------------*- C++ -*-==//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 /// \file
getRegClassesForCopy(MachineInstr &I, const TargetInstrInfo &TII,
   // information.
   if (SrcRegBank != DstRegBank && (DstSize == 1 && SrcSize == 1))
     SrcSize = DstSize = 32;
 
   return {getMinClassForRegBank(SrcRegBank, SrcSize, true),
           getMinClassForRegBank(DstRegBank, DstSize, true)};
 }
 
+static bool selectDebugInstr(MachineInstr &I, MachineRegisterInfo &MRI,
+                             const RegisterBankInfo &RBI) {
+  bool Ignore = false;
+  for (MachineOperand &MO : I.operands()) {
+    if (!MO.isReg())
+      continue;
+    Register Reg = MO.getReg();
+    if (!Reg)
+      continue;
+    LLT Ty = MRI.getType(Reg);
+    // If there is physical register or virutal register that has been
+    // assigned to a register class, just bail out. This logic is aligned
+    // to RegBankSelect when setting register bank for debug instructions.
+    if (!Ty.isValid()) {
+      Ignore = true;
+      break;
+    }
+  }
+  if (Ignore)
+    return true;
+  for (MachineOperand &MO : I.operands()) {
+    if (!MO.isReg())
+      continue;
+    Register Reg = MO.getReg();
+    if (!Reg)
+      continue;
+    LLT Ty = MRI.getType(Reg);
+    const RegClassOrRegBank &RegClassOrBank = MRI.getRegClassOrRegBank(Reg);
+    const TargetRegisterClass *RC =
+        RegClassOrBank.dyn_cast<const TargetRegisterClass *>();
+    if (!RC) {
+      const RegisterBank &RB = *RegClassOrBank.get<const RegisterBank *>();
+      RC = getRegClassForTypeOnBank(Ty, RB);
+      if (!RC) {
+        LLVM_DEBUG(
+            dbgs() << "Warning: DBG_VALUE operand has unexpected size/bank\n");
+        break;
+      }
+    }
+    RBI.constrainGenericRegister(Reg, *RC, MRI);
+  }
+
+  return true;
+}
+
 static bool selectCopy(MachineInstr &I, const TargetInstrInfo &TII,
                        MachineRegisterInfo &MRI, const TargetRegisterInfo &TRI,
                        const RegisterBankInfo &RBI) {
   Register DstReg = I.getOperand(0).getReg();
   Register SrcReg = I.getOperand(1).getReg();
   const RegisterBank &DstRegBank = *RBI.getRegBank(DstReg, MRI, TRI);
   const RegisterBank &SrcRegBank = *RBI.getRegBank(SrcReg, MRI, TRI);
 
if (Opcode == TargetOpcode::PHI || Opcode == TargetOpcode::G_PHI) {
       I.setDesc(TII.get(TargetOpcode::PHI));
 
       return RBI.constrainGenericRegister(DefReg, *DefRC, MRI);
     }
 
     if (I.isCopy())
       return selectCopy(I, TII, MRI, TRI, RBI);
 
+    if (I.isDebugInstr())
+      return selectDebugInstr(I, MRI, RBI);
+
     return true;
   }
 
 
   if (I.getNumOperands() != I.getNumExplicitOperands()) {
     LLVM_DEBUG(
         dbgs() << "Generic instruction has unexpected implicit operands\n");
     return false;

llvm/lib/Target/X86/X86InstructionSelector.cpp

 //===- X86InstructionSelector.cpp -----------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 /// \file
private:
   bool selectAnyext(MachineInstr &I, MachineRegisterInfo &MRI,
                     MachineFunction &MF) const;
   bool selectCmp(MachineInstr &I, MachineRegisterInfo &MRI,
                  MachineFunction &MF) const;
   bool selectFCmp(MachineInstr &I, MachineRegisterInfo &MRI,
                   MachineFunction &MF) const;
   bool selectUadde(MachineInstr &I, MachineRegisterInfo &MRI,
                    MachineFunction &MF) const;
+  bool selectDebugInstr(MachineInstr &I, MachineRegisterInfo &MRI) const;
   bool selectCopy(MachineInstr &I, MachineRegisterInfo &MRI) const;
   bool selectUnmergeValues(MachineInstr &I, MachineRegisterInfo &MRI,
                            MachineFunction &MF);
   bool selectMergeValues(MachineInstr &I, MachineRegisterInfo &MRI,
                          MachineFunction &MF);
   bool selectInsert(MachineInstr &I, MachineRegisterInfo &MRI,
                     MachineFunction &MF) const;
   bool selectExtract(MachineInstr &I, MachineRegisterInfo &MRI,
static const TargetRegisterClass *getRegClassFromGRPhysReg(Register Reg) {
   if (X86::GR16RegClass.contains(Reg))
     return &X86::GR16RegClass;
   if (X86::GR8RegClass.contains(Reg))
     return &X86::GR8RegClass;
 
   llvm_unreachable("Unknown RegClass for PhysReg!");
 }
 
+bool X86InstructionSelector::selectDebugInstr(MachineInstr &I,
+                                              MachineRegisterInfo &MRI) const {
+  bool Ignore = false;
+  for (MachineOperand &MO : I.operands()) {
+    if (!MO.isReg())
+      continue;
+    Register Reg = MO.getReg();
+    if (!Reg)
+      continue;
+    LLT Ty = MRI.getType(Reg);
+    // If there is physical register or virutal register that has been
+    // assigned to a register class, just bail out. This logic is aligned
+    // to RegBankSelect when setting register bank for debug instructions.
+    if (!Ty.isValid()) {
+      Ignore = true;
+      break;
+    }
+  }
+  if (Ignore)
+    return true;
+  for (MachineOperand &MO : I.operands()) {
+    if (!MO.isReg())
+      continue;
+    Register Reg = MO.getReg();
+    if (!Reg)
+      continue;
+    LLT Ty = MRI.getType(Reg);
+    const RegClassOrRegBank &RegClassOrBank = MRI.getRegClassOrRegBank(Reg);
+    const TargetRegisterClass *RC =
+        RegClassOrBank.dyn_cast<const TargetRegisterClass *>();
+    if (!RC) {
+      const RegisterBank &RB = *RegClassOrBank.get<const RegisterBank *>();
+      RC = getRegClass(Ty, RB);
+      if (!RC) {
+        LLVM_DEBUG(
+            dbgs() << "Warning: DBG_VALUE operand has unexpected size/bank\n");
+        break;
+      }
+    }
+    RBI.constrainGenericRegister(Reg, *RC, MRI);
+  }
+
+  return true;
+}
+
 // Set X86 Opcode and constrain DestReg.
 bool X86InstructionSelector::selectCopy(MachineInstr &I,
                                         MachineRegisterInfo &MRI) const {
   Register DstReg = I.getOperand(0).getReg();
   const unsigned DstSize = RBI.getSizeInBits(DstReg, MRI, TRI);
   const RegisterBank &DstRegBank = *RBI.getRegBank(DstReg, MRI, TRI);
 
   Register SrcReg = I.getOperand(1).getReg();
if (!isPreISelGenericOpcode(Opcode)) {
     // Certain non-generic instructions also need some special handling.
 
     if (Opcode == TargetOpcode::LOAD_STACK_GUARD)
       return false;
 
     if (I.isCopy())
       return selectCopy(I, MRI);
 
+    if (I.isDebugInstr())
+      return selectDebugInstr(I, MRI);
+
     return true;
   }
 
   assert(I.getNumOperands() == I.getNumExplicitOperands() &&
          "Generic instruction has unexpected implicit operands\n");
 
   if (selectImpl(I, *CoverageInfo))
     return true;

llvm/lib/Target/X86/X86RegisterBankInfo.cpp

 //===- X86RegisterBankInfo.cpp -----------------------------------*- C++ -*-==//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 /// \file

 
 void X86RegisterBankInfo::getInstrPartialMappingIdxs(
     const MachineInstr &MI, const MachineRegisterInfo &MRI, const bool isFP,
     SmallVectorImpl<PartialMappingIdx> &OpRegBankIdx) {
 
   unsigned NumOperands = MI.getNumOperands();
   for (unsigned Idx = 0; Idx < NumOperands; ++Idx) {
     auto &MO = MI.getOperand(Idx);
-    if (!MO.isReg())
+    if (!MO.isReg() || !MO.getReg())
       OpRegBankIdx[Idx] = PMI_None;
     else
       OpRegBankIdx[Idx] = getPartialMappingIdx(MRI.getType(MO.getReg()), isFP);
   }
 }
 
 bool X86RegisterBankInfo::getInstrValueMapping(
     const MachineInstr &MI,
     const SmallVectorImpl<PartialMappingIdx> &OpRegBankIdx,
     SmallVectorImpl<const ValueMapping *> &OpdsMapping) {
 
   unsigned NumOperands = MI.getNumOperands();
   for (unsigned Idx = 0; Idx < NumOperands; ++Idx) {
     if (!MI.getOperand(Idx).isReg())
       continue;
+    if (!MI.getOperand(Idx).getReg())
+      continue;
 
     auto Mapping = getValueMapping(OpRegBankIdx[Idx], 1);
     if (!Mapping->isValid())
       return false;
 
     OpdsMapping[Idx] = Mapping;
   }
   return true;

llvm/test/CodeGen/AArch64/GlobalISel/select-dbg-value.mir

 ---
 name:            test_dbg_value
 legalized:       true
 regBankSelected: true
 body: |
   bb.0:
     liveins: $w0
     ; CHECK-LABEL: name: test_dbg_value
-    ; CHECK: [[COPY:%[0-9]+]]:gpr32 = COPY $w0
-    ; CHECK: [[ADDWrr:%[0-9]+]]:gpr32 = ADDWrr [[COPY]], [[COPY]]
-    ; CHECK: $w0 = COPY [[ADDWrr]]
-    ; CHECK: DBG_VALUE [[ADDWrr]], $noreg, !7, !DIExpression(), debug-location !9
+    ; CHECK: liveins: $w0
+    ; CHECK-NEXT: {{  $}}
+    ; CHECK-NEXT: [[COPY:%[0-9]+]]:gpr32 = COPY $w0
+    ; CHECK-NEXT: [[ADDWrr:%[0-9]+]]:gpr32 = ADDWrr [[COPY]], [[COPY]]
+    ; CHECK-NEXT: $w0 = COPY [[ADDWrr]]
+    ; CHECK-NEXT: DBG_VALUE [[ADDWrr]], $noreg, !7, !DIExpression(), debug-location !9
     %0:gpr(s32) = COPY $w0
     %1:gpr(s32) = G_ADD %0, %0
     $w0 = COPY %1(s32)
     DBG_VALUE %1(s32), $noreg, !7, !DIExpression(), debug-location !9
 ...
 
 ---
 name:            test_dbg_value_dead
 legalized:       true
 regBankSelected: true
 body: |
   bb.0:
     liveins: $w0
     ; CHECK-LABEL: name: test_dbg_value_dead
-    ; CHECK-NOT: COPY
-    ; CHECK: DBG_VALUE %0:gpr, $noreg, !7, !DIExpression(), debug-location !9
+    ; CHECK: liveins: $w0
+    ; CHECK-NEXT: {{  $}}
+    ; CHECK-NEXT: DBG_VALUE %0:gpr32, $noreg, !7, !DIExpression(), debug-location !9
     %0:gpr(s32) = COPY $w0
     DBG_VALUE %0(s32), $noreg, !7, !DIExpression(), debug-location !9
 ...

llvm/test/CodeGen/AArch64/GlobalISel/select-hint.mir

 # NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
 # RUN: llc -mtriple aarch64 -debugify-and-strip-all-safe -run-pass=instruction-select -verify-machineinstrs %s -o - | FileCheck %s
 
 # Check that we remove hints during selection.
 
 ...
 ---
 name:            assert_zext_gpr
 legalized:       true
 regBankSelected: true
 tracksRegLiveness: true
 body:             |
   bb.0:
     liveins: $w0, $w1
 
     ; CHECK-LABEL: name: assert_zext_gpr
     ; CHECK: liveins: $w0, $w1
     ; CHECK-NEXT: {{  $}}
-    ; CHECK-NEXT: %copy:gpr32all = COPY $w0
+    ; CHECK-NEXT: %copy:gpr32 = COPY $w0
     ; CHECK-NEXT: $w1 = COPY %copy
     ; CHECK-NEXT: RET_ReallyLR implicit $w1
     %copy:gpr(s32) = COPY $w0
     %copy_assert_zext:gpr(s32) = G_ASSERT_ZEXT %copy, 16
     $w1 = COPY %copy_assert_zext(s32)
     RET_ReallyLR implicit $w1
 
 ...
 ---
 name:            assert_zext_fpr

 tracksRegLiveness: true
 body:             |
   bb.0:
     liveins: $w0, $w1
 
     ; CHECK-LABEL: name: assert_sext_gpr
     ; CHECK: liveins: $w0, $w1
     ; CHECK-NEXT: {{  $}}
-    ; CHECK-NEXT: %copy:gpr32all = COPY $w0
+    ; CHECK-NEXT: %copy:gpr32 = COPY $w0
     ; CHECK-NEXT: $w1 = COPY %copy
     ; CHECK-NEXT: RET_ReallyLR implicit $w1
     %copy:gpr(s32) = COPY $w0
     %copy_assert_sext:gpr(s32) = G_ASSERT_SEXT %copy, 16
     $w1 = COPY %copy_assert_sext(s32)
     RET_ReallyLR implicit $w1
 
 ...

llvm/test/DebugInfo/AArch64/debug-reg-bank.ll

 ; NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
 ; RUN: llc < %s -mtriple=aarch64-linux-gnu -O0 -global-isel -stop-after instruction-select | FileCheck %s
 
 define void @foo(i32 %n) !dbg !7 {
   ; CHECK-LABEL: name: foo
   ; CHECK: bb.1.entry:
   ; CHECK-NEXT:   liveins: $w0
   ; CHECK-NEXT: {{  $}}
-  ; CHECK-NEXT:   DBG_VALUE %0:_, $noreg, !12, !DIExpression(), debug-location !19
+  ; CHECK-NEXT:   DBG_VALUE %0:gpr32, $noreg, !12, !DIExpression(), debug-location !19
   ; CHECK-NEXT:   RET_ReallyLR debug-location !20
 entry:
   call void @llvm.dbg.value(metadata i32 %n, i64 0, metadata !12, metadata !19), !dbg !20
   ret void, !dbg !21
 }
 
 declare void @llvm.dbg.value(metadata, i64, metadata, metadata)
 

llvm/test/DebugInfo/X86/debug-reg-bank.ll

 ; NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
 ; RUN: llc < %s -mtriple=x86_64-linux-gnu -O0 -global-isel -stop-after instruction-select | FileCheck %s
 
 define void @foo(i32 %n) !dbg !7 {
   ; CHECK-LABEL: name: foo
   ; CHECK: bb.1.entry:
   ; CHECK-NEXT:   liveins: $edi
   ; CHECK-NEXT: {{  $}}
-  ; CHECK-NEXT:   DBG_VALUE %0:gpr, $noreg, !12, !DIExpression(), debug-location !19
+  ; CHECK-NEXT:   DBG_VALUE %0:gr32, $noreg, !12, !DIExpression(), debug-location !19
   ; CHECK-NEXT:   RET 0, debug-location !20
 entry:
   call void @llvm.dbg.value(metadata i32 %n, i64 0, metadata !12, metadata !19), !dbg !20
   ret void, !dbg !21
 }
 
 declare void @llvm.dbg.value(metadata, i64, metadata, metadata)