MachineVerifier: Verify REG_SEQUENCE

[arsenm]

Summary

Somehow there was no verification of this, other than an ad-hoc
assertion in TwoAddressInstructions.

[qcolombet]

Good catch @arsenm!

Overall LGTM. Just one missing case if I'm not mistaken.

llvm/lib/CodeGen/MachineVerifier.cpp

1940

Nit: For compactness, I would put MI->getOperand(I + 1) in its own variable.

llvm/test/MachineVerifier/verify-reg-sequence.mir

14

I think we miss a case in the test and in the verifier:

%val = REG_SEQUENCE <noOperands>

[arsenm]

94ebd7d9ff1776bbc94ca6ac82a783fa9d4eaa72

llvm/test/MachineVerifier/verify-reg-sequence.mir

14

This one was free

llvm/lib/CodeGen/MachineOperand.cpp

if (IsFixed)
       FrameIndex -= MFI->getObjectIndexBegin();
   }
   MachineOperand::printStackObjectReference(OS, FrameIndex, IsFixed, Name);
 }
 
 void MachineOperand::printSubRegIdx(raw_ostream &OS, uint64_t Index,
                                     const TargetRegisterInfo *TRI) {
   OS << "%subreg.";
-  if (TRI)
+  if (TRI && Index != 0 && Index < TRI->getNumSubRegIndices())
     OS << TRI->getSubRegIndexName(Index);
   else
     OS << Index;
 }
 
 void MachineOperand::printTargetFlags(raw_ostream &OS,
                                       const MachineOperand &Op) {
   if (!Op.getTargetFlags())

llvm/lib/CodeGen/MachineVerifier.cpp

else
       InsertedSize = TRI->getRegSizeInBits(MI->getOperand(2).getReg(), *MRI);
     unsigned SubRegSize = TRI->getSubRegIdxSize(MI->getOperand(3).getImm());
     if (SubRegSize < InsertedSize) {
       report("INSERT_SUBREG expected inserted value to have equal or lesser "
              "size than the subreg it was inserted into", MI);
       break;
     }
   } break;
+  case TargetOpcode::REG_SEQUENCE: {
+    unsigned NumOps = MI->getNumOperands();
+    if (!(NumOps & 1)) {
+      report("Invalid number of operands for REG_SEQUENCE", MI);
+      break;
+    }
+
+    for (unsigned I = 1; I != NumOps; I += 2) {
+      if (!MI->getOperand(I).isReg())
+        report("Invalid register operand for REG_SEQUENCE", &MI->getOperand(I),
+               I);
+
+      if (!MI->getOperand(I + 1).isImm() ||
+          MI->getOperand(I + 1).getImm() == 0 ||
+          MI->getOperand(I + 1).getImm() >= TRI->getNumSubRegIndices()) {
+        report("Invalid subregister index operand for REG_SEQUENCE",
+               &MI->getOperand(I + 1), I + 1);
+      }
+    }
+
+    Register DstReg = MI->getOperand(0).getReg();
+    if (DstReg.isPhysical())
+      report("REG_SEQUENCE does not support physical register results", MI);
+
+    if (MI->getOperand(0).getSubReg())
+      report("Invalid subreg result for REG_SEQUENCE", MI);
+
+    break;
+  }
   }
 }
 
 void
 MachineVerifier::visitMachineOperand(const MachineOperand *MO, unsigned MONum) {
   const MachineInstr *MI = MO->getParent();
   const MCInstrDesc &MCID = MI->getDesc();
   unsigned NumDefs = MCID.getNumDefs();

llvm/lib/CodeGen/TwoAddressInstructionPass.cpp

 /// Becomes:
 ///
 ///   undef %dst:ssub0 = COPY %v1
 ///   %dst:ssub1 = COPY %v2
 void TwoAddressInstructionPass::
 eliminateRegSequence(MachineBasicBlock::iterator &MBBI) {
   MachineInstr &MI = *MBBI;
   Register DstReg = MI.getOperand(0).getReg();
-  if (MI.getOperand(0).getSubReg() || DstReg.isPhysical() ||
-      !(MI.getNumOperands() & 1)) {
-    LLVM_DEBUG(dbgs() << "Illegal REG_SEQUENCE instruction:" << MI);
-    llvm_unreachable(nullptr);
-  }
 
   SmallVector<Register, 4> OrigRegs;
   if (LIS) {
     OrigRegs.push_back(MI.getOperand(0).getReg());
     for (unsigned i = 1, e = MI.getNumOperands(); i < e; i += 2)
       OrigRegs.push_back(MI.getOperand(i).getReg());
   }
 

llvm/test/CodeGen/AMDGPU/load-store-opt-scc.mir

bb.0 (%ir-block.0):
 
     $sgpr2 = S_MOV_B32 -1
     $sgpr3 = S_MOV_B32 61440
 
     %0:sgpr_64 = COPY $sgpr0_sgpr1
     %1:sgpr_64 = S_LOAD_DWORDX2_IMM %1, 36, 0 :: (dereferenceable invariant load (s64) from `i64 addrspace(4)* undef`, addrspace 4)
     %2:sgpr_32 = COPY $sgpr2
     %3:sgpr_32 = COPY $sgpr3
-    %4:sgpr_128 = REG_SEQUENCE %1, %2, %3
+    %4:sgpr_128 = REG_SEQUENCE %1, %subreg.sub0, %2, %subreg.sub1, %3, %subreg.sub2
 
     %5:vgpr_32 = COPY $vgpr0
     %6:vgpr_32 = COPY $vgpr1
 
     BUFFER_STORE_DWORD_OFFSET %5, %4, 0, 4, 0, 0, 0, implicit $exec :: (store (s32) into %ir.out.gep.1, addrspace 1)
     BUFFER_STORE_DWORD_OFFSET %6, %4, 0, 8, 0, 0, 0, implicit $exec :: (store (s32) into %ir.out.gep.1, addrspace 1)
 
     S_ENDPGM 0
bb.0 (%ir-block.0):
 
     $sgpr2 = S_MOV_B32 -1
     $sgpr3 = S_MOV_B32 61440
 
     %0:sgpr_64 = COPY $sgpr0_sgpr1
     %1:sgpr_64 = S_LOAD_DWORDX2_IMM %1, 36, 0 :: (dereferenceable invariant load (s64) from `i64 addrspace(4)* undef`, addrspace 4)
     %2:sgpr_32 = COPY $sgpr2
     %3:sgpr_32 = COPY $sgpr3
-    %4:sgpr_128 = REG_SEQUENCE %1, %2, %3
+    %4:sgpr_128 = REG_SEQUENCE %1, %subreg.sub0, %2, %subreg.sub1, %3, %subreg.sub2
 
     %5:vgpr_32 = COPY $vgpr0
     %6:vgpr_32 = COPY $vgpr1
 
     BUFFER_STORE_DWORD_OFFSET %5, %4, 0, 4, 16, 0, 0, implicit $exec :: (store (s32) into %ir.out.gep.1, addrspace 1)
     BUFFER_STORE_DWORD_OFFSET %6, %4, 0, 8, 0, 0, 0, implicit $exec :: (store (s32) into %ir.out.gep.1, addrspace 1)
 
     S_ENDPGM 0
bb.0 (%ir-block.0):
 
     $sgpr2 = S_MOV_B32 -1
     $sgpr3 = S_MOV_B32 61440
 
     %0:sgpr_64 = COPY $sgpr0_sgpr1
     %1:sgpr_64 = S_LOAD_DWORDX2_IMM %1, 36, 0 :: (dereferenceable invariant load (s64) from `i64 addrspace(4)* undef`, addrspace 4)
     %2:sgpr_32 = COPY $sgpr2
     %3:sgpr_32 = COPY $sgpr3
-    %4:sgpr_128 = REG_SEQUENCE %1, %2, %3
+    %4:sgpr_128 = REG_SEQUENCE %1, %subreg.sub0, %2, %subreg.sub1, %3, %subreg.sub2
 
     %5:vgpr_32 = COPY $vgpr0
     %6:vgpr_32 = COPY $vgpr1
 
     BUFFER_STORE_DWORD_OFFSET %5, %4, 0, 4, 0, 0, 0, implicit $exec :: (store (s32) into %ir.out.gep.1, addrspace 1)
     BUFFER_STORE_DWORD_OFFSET %6, %4, 0, 8, 16, 0, 0, implicit $exec :: (store (s32) into %ir.out.gep.1, addrspace 1)
 
     S_ENDPGM 0
bb.0 (%ir-block.0):
 
     $sgpr2 = S_MOV_B32 -1
     $sgpr3 = S_MOV_B32 61440
 
     %0:sgpr_64 = COPY $sgpr0_sgpr1
     %1:sgpr_64 = S_LOAD_DWORDX2_IMM %1, 36, 0 :: (dereferenceable invariant load (s64) from `i64 addrspace(4)* undef`, addrspace 4)
     %2:sgpr_32 = COPY $sgpr2
     %3:sgpr_32 = COPY $sgpr3
-    %4:sgpr_128 = REG_SEQUENCE %1, %2, %3
+    %4:sgpr_128 = REG_SEQUENCE %1, %subreg.sub0, %2, %subreg.sub1, %3, %subreg.sub2
 
     %5:vgpr_32 = COPY $vgpr0
     %6:vgpr_32 = COPY $vgpr1
 
     BUFFER_STORE_DWORD_OFFSET %5, %4, 0, 4, 16, 0, 0, implicit $exec :: (store (s32) into %ir.out.gep.1, addrspace 1)
     BUFFER_STORE_DWORD_OFFSET %6, %4, 0, 8, 16, 0, 0, implicit $exec :: (store (s32) into %ir.out.gep.1, addrspace 1)
 
     S_ENDPGM 0
 ...

llvm/test/CodeGen/MIR/AMDGPU/load-store-opt-dlc.mir

bb.0 (%ir-block.0):
 
     $sgpr2 = S_MOV_B32 -1
     $sgpr3 = S_MOV_B32 61440
 
     %0:sgpr_64 = COPY $sgpr0_sgpr1
     %1:sgpr_64 = S_LOAD_DWORDX2_IMM %1, 36, 0 :: (dereferenceable invariant load (s64) from `i64 addrspace(4)* undef`, addrspace 4)
     %2:sgpr_32 = COPY $sgpr2
     %3:sgpr_32 = COPY $sgpr3
-    %4:sgpr_128 = REG_SEQUENCE %1, %2, %3
+    %4:sgpr_128 = REG_SEQUENCE %1, %subreg.sub0, %2, %subreg.sub1, %3, %subreg.sub2
 
     %5:vgpr_32 = COPY $vgpr0
     %6:vgpr_32 = COPY $vgpr1
 
     BUFFER_STORE_DWORD_OFFSET %5, %4, 0, 4, 0, 0, 0, implicit $exec :: (store (s32) into %ir.out.gep.1, addrspace 1)
     BUFFER_STORE_DWORD_OFFSET %6, %4, 0, 8, 0, 0, 0, implicit $exec :: (store (s32) into %ir.out.gep.1, addrspace 1)
 
     S_ENDPGM 0
bb.0 (%ir-block.0):
 
     $sgpr2 = S_MOV_B32 -1
     $sgpr3 = S_MOV_B32 61440
 
     %0:sgpr_64 = COPY $sgpr0_sgpr1
     %1:sgpr_64 = S_LOAD_DWORDX2_IMM %1, 36, 0 :: (dereferenceable invariant load (s64) from `i64 addrspace(4)* undef`, addrspace 4)
     %2:sgpr_32 = COPY $sgpr2
     %3:sgpr_32 = COPY $sgpr3
-    %4:sgpr_128 = REG_SEQUENCE %1, %2, %3
+    %4:sgpr_128 = REG_SEQUENCE %1, %subreg.sub0, %2, %subreg.sub1, %3, %subreg.sub2
 
     %5:vgpr_32 = COPY $vgpr0
     %6:vgpr_32 = COPY $vgpr1
 
     BUFFER_STORE_DWORD_OFFSET %5, %4, 0, 4, 4, 0, 0, implicit $exec :: (store (s32) into %ir.out.gep.1, addrspace 1)
     BUFFER_STORE_DWORD_OFFSET %6, %4, 0, 8, 0, 0, 0, implicit $exec :: (store (s32) into %ir.out.gep.1, addrspace 1)
 
     S_ENDPGM 0
bb.0 (%ir-block.0):
 
     $sgpr2 = S_MOV_B32 -1
     $sgpr3 = S_MOV_B32 61440
 
     %0:sgpr_64 = COPY $sgpr0_sgpr1
     %1:sgpr_64 = S_LOAD_DWORDX2_IMM %1, 36, 0 :: (dereferenceable invariant load (s64) from `i64 addrspace(4)* undef`, addrspace 4)
     %2:sgpr_32 = COPY $sgpr2
     %3:sgpr_32 = COPY $sgpr3
-    %4:sgpr_128 = REG_SEQUENCE %1, %2, %3
+    %4:sgpr_128 = REG_SEQUENCE %1, %subreg.sub0, %2, %subreg.sub1, %3, %subreg.sub2
 
     %5:vgpr_32 = COPY $vgpr0
     %6:vgpr_32 = COPY $vgpr1
 
     BUFFER_STORE_DWORD_OFFSET %5, %4, 0, 4, 0, 0, 0, implicit $exec :: (store (s32) into %ir.out.gep.1, addrspace 1)
     BUFFER_STORE_DWORD_OFFSET %6, %4, 0, 8, 4, 0, 0, implicit $exec :: (store (s32) into %ir.out.gep.1, addrspace 1)
 
     S_ENDPGM 0
bb.0 (%ir-block.0):
 
     $sgpr2 = S_MOV_B32 -1
     $sgpr3 = S_MOV_B32 61440
 
     %0:sgpr_64 = COPY $sgpr0_sgpr1
     %1:sgpr_64 = S_LOAD_DWORDX2_IMM %1, 36, 0 :: (dereferenceable invariant load (s64) from `i64 addrspace(4)* undef`, addrspace 4)
     %2:sgpr_32 = COPY $sgpr2
     %3:sgpr_32 = COPY $sgpr3
-    %4:sgpr_128 = REG_SEQUENCE %1, %2, %3
+    %4:sgpr_128 = REG_SEQUENCE %1, %subreg.sub0, %2, %subreg.sub1, %3, %subreg.sub2
 
     %5:vgpr_32 = COPY $vgpr0
     %6:vgpr_32 = COPY $vgpr1
 
     BUFFER_STORE_DWORD_OFFSET %5, %4, 0, 4, 4, 0, 0, implicit $exec :: (store (s32) into %ir.out.gep.1, addrspace 1)
     BUFFER_STORE_DWORD_OFFSET %6, %4, 0, 8, 4, 0, 0, implicit $exec :: (store (s32) into %ir.out.gep.1, addrspace 1)
 
     S_ENDPGM 0
 ...

llvm/test/CodeGen/MIR/X86/subregister-index-operands.mir

 registers:
   - { id: 0, class: gr32 }
   - { id: 1, class: gr8 }
 body: |
   bb.0.entry:
     liveins: $edi, $eax
     ; CHECK-LABEL: name: t
     ; CHECK: liveins: $edi, $eax
-    ; CHECK: [[INSERT_SUBREG:%[0-9]+]]:gr32 = INSERT_SUBREG $edi, $al, %subreg.sub_8bit
-    ; CHECK: [[EXTRACT_SUBREG:%[0-9]+]]:gr8 = EXTRACT_SUBREG $eax, %subreg.sub_8bit_hi
-    ; CHECK: $ax = REG_SEQUENCE [[EXTRACT_SUBREG]], %subreg.sub_8bit, [[EXTRACT_SUBREG]], %subreg.sub_8bit_hi
-    ; CHECK: RET64 $ax
+    ; CHECK-NEXT: {{  $}}
+    ; CHECK-NEXT: [[INSERT_SUBREG:%[0-9]+]]:gr32 = INSERT_SUBREG $edi, $al, %subreg.sub_8bit
+    ; CHECK-NEXT: [[EXTRACT_SUBREG:%[0-9]+]]:gr8 = EXTRACT_SUBREG $eax, %subreg.sub_8bit_hi
+    ; CHECK-NEXT: [[REG_SEQUENCE:%[0-9]+]]:gr8 = REG_SEQUENCE [[EXTRACT_SUBREG]], %subreg.sub_8bit, [[EXTRACT_SUBREG]], %subreg.sub_8bit_hi
+    ; CHECK-NEXT: $ax = COPY [[REG_SEQUENCE]]
+    ; CHECK-NEXT: RET64 $ax
     %0 = INSERT_SUBREG $edi, $al, %subreg.sub_8bit
     %1 = EXTRACT_SUBREG $eax, %subreg.sub_8bit_hi
-    $ax = REG_SEQUENCE %1, %subreg.sub_8bit, %1, %subreg.sub_8bit_hi
+    %2:gr8 = REG_SEQUENCE %1, %subreg.sub_8bit, %1, %subreg.sub_8bit_hi
+    $ax = COPY %2
     RET64 $ax
 ...
 

llvm/test/MachineVerifier/verify-reg-sequence.mir

+# REQUIRES: amdgpu-registered-target
+# RUN: not --crash llc -mtriple=amdgcn-amd-amdhsa -mcpu=gfx900 -run-pass=none -o /dev/null %s 2>&1 | FileCheck %s
+
+---
+name: invalid_reg_sequence
+tracksRegLiveness: true
+body:             |
+  bb.0:
+    %0:vgpr_32 = IMPLICIT_DEF
+    %1:vgpr_32 = IMPLICIT_DEF
+
+    ; No operands
+    ; CHECK: *** Bad machine code: Too few operands ***
+    REG_SEQUENCE
+
+    ; Missing destination
+    ; CHECK: *** Bad machine code: Explicit definition marked as use ***
+    REG_SEQUENCE %0, %subreg.sub0, %1, %subreg.sub1
+
+    ; Missing subreg operand
+    ; CHECK: *** Bad machine code: Invalid number of operands for REG_SEQUENCE ***
+    %2:vreg_64 = REG_SEQUENCE %0, %subreg.sub0, %1
+
+    ; Missing register operand
+    ; CHECK: *** Bad machine code: Invalid number of operands for REG_SEQUENCE ***
+    %2:vreg_64 = REG_SEQUENCE %0, %subreg.sub0, %subreg.sub1
+
+    ; Physreg destination
+    ; CHECK: *** Bad machine code: REG_SEQUENCE does not support physical register results ***
+    $vgpr0_vgpr1 = REG_SEQUENCE %0, %subreg.sub0, %1, %subreg.sub1
+
+    ; Subreg in destination
+    ; CHECK: *** Bad machine code: Invalid subreg result for REG_SEQUENCE ***
+    %3.sub0_sub1:vreg_128 = REG_SEQUENCE %0, %subreg.sub0, %1, %subreg.sub1
+
+    ; All operands are registers
+    ; CHECK: *** Bad machine code: Invalid subregister index operand for REG_SEQUENCE ***
+    %4:vreg_64 = REG_SEQUENCE %0, %1
+
+    ; Register and subreg index operand order swapped
+    ; CHECK: *** Bad machine code: Invalid register operand for REG_SEQUENCE ***
+    ; CHECK: *** Bad machine code: Invalid subregister index operand for REG_SEQUENCE ***
+    %5:vreg_64 = REG_SEQUENCE %subreg.sub0, %0, %subreg.sub1, %1
+
+    ; Invalid subreg index constants
+   ; CHECK: *** Bad machine code: Invalid subregister index operand for REG_SEQUENCE ***
+   ; CHECK: - instruction: %6:vreg_64 = REG_SEQUENCE %0:vgpr_32, %subreg.0, %1:vgpr_32, %subreg.99999
+   ; CHECK-NEXT: operand 2:   0
+
+    ; CHECK: *** Bad machine code: Invalid subregister index operand for REG_SEQUENCE ***
+    ; CHECK: instruction: %6:vreg_64 = REG_SEQUENCE %0:vgpr_32, %subreg.0, %1:vgpr_32, %subreg.99999
+    ; CHECK-NEXT: operand 4:   99999
+    %6:vreg_64 = REG_SEQUENCE %0, 0, %1, 99999
+
+...