[AArch64][GlobalISel] Use getRegClassForTypeOnBank() in selectCopy.

[aemerson]

Summary

There were a lot of test failures (but unknown if they're really indicative of an issue) if I tried to adapt selectCopy to use the helper such that it used GPR64 rather than GPR64all, so I added a parameter to the helper to maintain the existing behavior. It seems reasonable to me to be able to COPY from XZR or SP so my guess is that the existing behavior was correct.

As for why it wasn't using it in the first place, the helper was added after selectCopy() so it was just probably an oversight.

[qcolombet]

LGTM.

One question below.

lib/Target/AArch64/AArch64InstructionSelector.cpp

139 ↗

(On Diff #132527)

Would it result in incorrect code if we were to always take the "all" variant?

[aemerson]

lib/Target/AArch64/AArch64InstructionSelector.cpp

139 ↗

(On Diff #132527)

I've discussed this with Tim who originally added this function, he thinks that we should probably keep the existing behavior and not force users to use the "all" RC. In practice I don't think it matters too much which one we pick (most of the time).

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

 #undef GET_GLOBALISEL_TEMPORARIES_INIT
 {
 }
 
 // FIXME: This should be target-independent, inferred from the types declared
 // for each class in the bank.
 static const TargetRegisterClass *
 getRegClassForTypeOnBank(LLT Ty, const RegisterBank &RB,
-                         const RegisterBankInfo &RBI) {
+                         const RegisterBankInfo &RBI,
+                         bool GetAllRegSet = false) {
   if (RB.getID() == AArch64::GPRRegBankID) {
     if (Ty.getSizeInBits() <= 32)
-      return &AArch64::GPR32RegClass;
+      return GetAllRegSet ? &AArch64::GPR32allRegClass
+                          : &AArch64::GPR32RegClass;
     if (Ty.getSizeInBits() == 64)
-      return &AArch64::GPR64RegClass;
+      return GetAllRegSet ? &AArch64::GPR64allRegClass
+                          : &AArch64::GPR64RegClass;
     return nullptr;
   }
 
   if (RB.getID() == AArch64::FPRRegBankID) {
+    if (Ty.getSizeInBits() <= 16)
+      return &AArch64::FPR16RegClass;
     if (Ty.getSizeInBits() == 32)
       return &AArch64::FPR32RegClass;
     if (Ty.getSizeInBits() == 64)
       return &AArch64::FPR64RegClass;
     if (Ty.getSizeInBits() == 128)
       return &AArch64::FPR128RegClass;
     return nullptr;
   }
static bool selectCopy(MachineInstr &I, const TargetInstrInfo &TII,
   unsigned DstReg = I.getOperand(0).getReg();
   if (TargetRegisterInfo::isPhysicalRegister(DstReg)) {
     assert(I.isCopy() && "Generic operators do not allow physical registers");
     return true;
   }
 
   const RegisterBank &RegBank = *RBI.getRegBank(DstReg, MRI, TRI);
   const unsigned DstSize = MRI.getType(DstReg).getSizeInBits();
+  (void)DstSize;
   unsigned SrcReg = I.getOperand(1).getReg();
   const unsigned SrcSize = RBI.getSizeInBits(SrcReg, MRI, TRI);
   (void)SrcSize;
   assert((!TargetRegisterInfo::isPhysicalRegister(SrcReg) || I.isCopy()) &&
          "No phys reg on generic operators");
   assert(
       (DstSize == SrcSize ||
        // Copies are a mean to setup initial types, the number of
        // bits may not exactly match.
        (TargetRegisterInfo::isPhysicalRegister(SrcReg) &&
         DstSize <= RBI.getSizeInBits(SrcReg, MRI, TRI)) ||
        // Copies are a mean to copy bits around, as long as we are
        // on the same register class, that's fine. Otherwise, that
        // means we need some SUBREG_TO_REG or AND & co.
        (((DstSize + 31) / 32 == (SrcSize + 31) / 32) && DstSize > SrcSize)) &&
       "Copy with different width?!");
   assert((DstSize <= 64 || RegBank.getID() == AArch64::FPRRegBankID) &&
          "GPRs cannot get more than 64-bit width values");
-  const TargetRegisterClass *RC = nullptr;
 
-  if (RegBank.getID() == AArch64::FPRRegBankID) {
-    if (DstSize <= 16)
-      RC = &AArch64::FPR16RegClass;
-    else if (DstSize <= 32)
-      RC = &AArch64::FPR32RegClass;
-    else if (DstSize <= 64)
-      RC = &AArch64::FPR64RegClass;
-    else if (DstSize <= 128)
-      RC = &AArch64::FPR128RegClass;
-    else {
-      DEBUG(dbgs() << "Unexpected bitcast size " << DstSize << '\n');
-      return false;
-    }
-  } else {
-    assert(RegBank.getID() == AArch64::GPRRegBankID &&
-           "Bitcast for the flags?");
-    RC =
-        DstSize <= 32 ? &AArch64::GPR32allRegClass : &AArch64::GPR64allRegClass;
-  }
+  const TargetRegisterClass *RC = getRegClassForTypeOnBank(
+      MRI.getType(DstReg), RegBank, RBI, /* GetAllRegSet */ true);
+  if (!RC) {
 
   // No need to constrain SrcReg. It will get constrained when
   // we hit another of its use or its defs.
   // Copies do not have constraints.
   if (!RBI.constrainGenericRegister(DstReg, *RC, MRI)) {
     DEBUG(dbgs() << "Failed to constrain " << TII.getName(I.getOpcode())
                  << " operand\n");
     return false;