GlobalISel: Relax verification of physical register copy types

[arsenm]

Summary

This was picking a concrete size for a physical register, and
enforcing exact match on the virtual register's type size. Some
targets add multiple types to a register class, and some are smaller
than the full bit width. For example x86 adds f32 to 128-bit xmm
registers, and AMDGPU adds i16/f16 to 32-bit registers.

It might be better to represent these cases as a copy of the full
register and an extraction of the subpart, but a lot of code assumes
you can directly copy. This will help fix the current usage of the DAG
calling convention infrastructure which is incompatible with how
GlobalISel is now using it.

The API is somewhat cumbersome here, but I just mirrored the existing
functions, except now with LLTs (and allow returning null on failure,
unlike the MVT version). I think the concept of selecting register
classes based on type is flawed to begin with, but I'm trying to keep
this compatible with the existing handling.

[arsenm]

ping

[qcolombet]

LGTM.

[arsenm]

cea97fc0fcd86d52f9efa215116356b72faeb17d

llvm/include/llvm/CodeGen/TargetRegisterInfo.h

public:
   /// Return true if the given TargetRegisterClass has the ValueType T.
   bool isTypeLegalForClass(const TargetRegisterClass &RC, MVT T) const {
     for (auto I = legalclasstypes_begin(RC); *I != MVT::Other; ++I)
       if (MVT(*I) == T)
         return true;
     return false;
   }
 
+  /// Return true if the given TargetRegisterClass is compatible with LLT T.
+  bool isTypeLegalForClass(const TargetRegisterClass &RC, LLT T) const {
+    for (auto I = legalclasstypes_begin(RC); *I != MVT::Other; ++I) {
+      MVT VT(*I);
+      if (VT == MVT::Untyped)
+        return true;
+
+      if (LLT(VT) == T)
+        return true;
+    }
+    return false;
+  }
+
   /// Loop over all of the value types that can be represented by values
   /// in the given register class.
   vt_iterator legalclasstypes_begin(const TargetRegisterClass &RC) const {
     return getRegClassInfo(RC).VTList;
   }
 
   vt_iterator legalclasstypes_end(const TargetRegisterClass &RC) const {
     vt_iterator I = legalclasstypes_begin(RC);
     while (*I != MVT::Other)
       ++I;
     return I;
   }
 
   /// Returns the Register Class of a physical register of the given type,
   /// picking the most sub register class of the right type that contains this
   /// physreg.
   const TargetRegisterClass *getMinimalPhysRegClass(MCRegister Reg,
                                                     MVT VT = MVT::Other) const;
 
+  /// Returns the Register Class of a physical register of the given type,
+  /// picking the most sub register class of the right type that contains this
+  /// physreg. If there is no register class compatible with the given type,
+  /// returns nullptr.
+  const TargetRegisterClass *getMinimalPhysRegClassLLT(MCRegister Reg,
+                                                       LLT Ty = LLT()) const;
+
   /// Return the maximal subclass of the given register class that is
   /// allocatable or NULL.
   const TargetRegisterClass *
     getAllocatableClass(const TargetRegisterClass *RC) const;
 
   /// Returns a bitset indexed by register number indicating if a register is
   /// allocatable or not. If a register class is specified, returns the subset
   /// for the class.

llvm/lib/CodeGen/MachineVerifier.cpp

void MachineVerifier::visitMachineInstrBefore(const MachineInstr *MI) {
   if (!TII->verifyInstruction(*MI, ErrorInfo))
     report(ErrorInfo.data(), MI);
 
   // Verify properties of various specific instruction types
   switch (MI->getOpcode()) {
   case TargetOpcode::COPY: {
     const MachineOperand &DstOp = MI->getOperand(0);
     const MachineOperand &SrcOp = MI->getOperand(1);
-    LLT DstTy = MRI->getType(DstOp.getReg());
-    LLT SrcTy = MRI->getType(SrcOp.getReg());
+    const Register SrcReg = SrcOp.getReg();
+    const Register DstReg = DstOp.getReg();
+
+    LLT DstTy = MRI->getType(DstReg);
+    LLT SrcTy = MRI->getType(SrcReg);
     if (SrcTy.isValid() && DstTy.isValid()) {
       // If both types are valid, check that the types are the same.
       if (SrcTy != DstTy) {
         report("Copy Instruction is illegal with mismatching types", MI);
         errs() << "Def = " << DstTy << ", Src = " << SrcTy << "\n";
       }
+
+      break;
+    }
+
+    if (!SrcTy.isValid() && !DstTy.isValid())
+      break;
+
+    // If we have only one valid type, this is likely a copy between a virtual
+    // and physical register.
+    unsigned SrcSize = 0;
+    unsigned DstSize = 0;
+    if (SrcReg.isPhysical() && DstTy.isValid()) {
+      const TargetRegisterClass *SrcRC =
+          TRI->getMinimalPhysRegClassLLT(SrcReg, DstTy);
+      if (SrcRC)
+        SrcSize = TRI->getRegSizeInBits(*SrcRC);
+    }
+
+    if (SrcSize == 0)
+      SrcSize = TRI->getRegSizeInBits(SrcReg, *MRI);
+
+    if (DstReg.isPhysical() && SrcTy.isValid()) {
+      const TargetRegisterClass *DstRC =
+          TRI->getMinimalPhysRegClassLLT(DstReg, SrcTy);
+      if (DstRC)
+        DstSize = TRI->getRegSizeInBits(*DstRC);
     }
-    if (SrcTy.isValid() || DstTy.isValid()) {
-      // If one of them have valid types, let's just check they have the same
-      // size.
-      unsigned SrcSize = TRI->getRegSizeInBits(SrcOp.getReg(), *MRI);
-      unsigned DstSize = TRI->getRegSizeInBits(DstOp.getReg(), *MRI);
-      assert(SrcSize && "Expecting size here");
-      assert(DstSize && "Expecting size here");
-      if (SrcSize != DstSize)
-        if (!DstOp.getSubReg() && !SrcOp.getSubReg()) {
-          report("Copy Instruction is illegal with mismatching sizes", MI);
-          errs() << "Def Size = " << DstSize << ", Src Size = " << SrcSize
-                 << "\n";
-        }
+
+    if (DstSize == 0)
+      DstSize = TRI->getRegSizeInBits(DstReg, *MRI);
+
+    if (SrcSize != 0 && DstSize != 0 && SrcSize != DstSize) {
     }
     break;
   }
   case TargetOpcode::STATEPOINT: {
     StatepointOpers SO(MI);
     if (!MI->getOperand(SO.getIDPos()).isImm() ||
         !MI->getOperand(SO.getNBytesPos()).isImm() ||
         !MI->getOperand(SO.getNCallArgsPos()).isImm()) {

llvm/lib/CodeGen/TargetRegisterInfo.cpp

if ((VT == MVT::Other || isTypeLegalForClass(*RC, VT)) &&
         RC->contains(reg) && (!BestRC || BestRC->hasSubClass(RC)))
       BestRC = RC;
   }
 
   assert(BestRC && "Couldn't find the register class");
   return BestRC;
 }
 
+const TargetRegisterClass *
+TargetRegisterInfo::getMinimalPhysRegClassLLT(MCRegister reg, LLT Ty) const {
+  assert(Register::isPhysicalRegister(reg) &&
+         "reg must be a physical register");
+
+  // Pick the most sub register class of the right type that contains
+  // this physreg.
+  const TargetRegisterClass *BestRC = nullptr;
+  for (const TargetRegisterClass *RC : regclasses()) {
+    if ((!Ty.isValid() || isTypeLegalForClass(*RC, Ty)) && RC->contains(reg) &&
+        (!BestRC || BestRC->hasSubClass(RC)))
+      BestRC = RC;
+  }
+
+  return BestRC;
+}
+
 /// getAllocatableSetForRC - Toggle the bits that represent allocatable
 /// registers for the specific register class.
 static void getAllocatableSetForRC(const MachineFunction &MF,
                                    const TargetRegisterClass *RC, BitVector &R){
   assert(RC->isAllocatable() && "invalid for nonallocatable sets");
   ArrayRef<MCPhysReg> Order = RC->getRawAllocationOrder(MF);
   for (unsigned i = 0; i != Order.size(); ++i)
     R.set(Order[i]);

llvm/test/MachineVerifier/test_copy_physregs_x86.mir

+#RUN: not --crash llc -march=x86-64 -run-pass=none -verify-machineinstrs -o /dev/null %s 2>&1 | FileCheck %s
+# REQUIRES: x86-registered-target
+
+# These copies have mismatched type sizes that are allowed because the
+# register class is defined to directly include the narrower type.
+---
+name:            test_valid_copies
+tracksRegLiveness: true
+body:             |
+  bb.0:
+    liveins: $xmm0, $xmm1, $xmm2, $xmm3, $xmm4
+    %0:_(s32) = COPY $xmm0
+    %1:_(s64) = COPY $xmm1
+    %2:_(s128) = COPY $xmm2
+    %3:_(<4 x s32>) = COPY $xmm3
+    %4:_(<2 x s64>) = COPY $xmm4
+    $xmm0 = COPY %0
+    $xmm1 = COPY %1
+    $xmm2 = COPY %2
+    $xmm3 = COPY %3
+    $xmm4 = COPY %4
+...
+
+---
+name:            test_invalid_copies
+tracksRegLiveness: true
+body:             |
+  bb.0:
+    liveins: $xmm0, $xmm1, $xmm2, $xmm3
+
+    ; CHECK: *** Bad machine code: Copy Instruction is illegal with mismatching sizes ***
+    %0:_(s16) = COPY $xmm0
+
+    ; CHECK: *** Bad machine code: Copy Instruction is illegal with mismatching sizes ***
+    %1:_(<4 x s16>) = COPY $xmm1
+
+    ; CHECK: *** Bad machine code: Copy Instruction is illegal with mismatching sizes ***
+    %2:_(s256) = COPY $xmm2
+
+    ; CHECK: *** Bad machine code: Copy Instruction is illegal with mismatching sizes ***
+    %3:_(<8 x s32>) = COPY $xmm3
+
+    ; CHECK: *** Bad machine code: Copy Instruction is illegal with mismatching sizes ***
+    $xmm0 = COPY %0
+
+    ; CHECK: *** Bad machine code: Copy Instruction is illegal with mismatching sizes ***
+    $xmm1 = COPY %1
+
+    ; CHECK: *** Bad machine code: Copy Instruction is illegal with mismatching sizes ***
+    $xmm2 = COPY %2
+
+    ; CHECK: *** Bad machine code: Copy Instruction is illegal with mismatching sizes ***
+    $xmm3 = COPY %3
+...