[AArch64][GlobalISel] Generalize logic for promoting copies

[tambre]

Summary

Generalize the 16-bit FPR to 32-bit GPR logic to work for all cases where destination size is bigger than source size.

Also fixed CheckCopy() always returning true instead of the result of isValidCopy().

[tambre]

This actually breaks CodeGen/AArch64/f16-instructions.ll's test_select. Will investigate.
If you see anything incorrect in the current changes let me know. :)

[paquette]

llvm/lib/Target/AArch64/AArch64InstructionSelector.cpp

794–795

It's weird that we have this case and also the case above which checks getMinSizeForRegBank(SrcRegBank) > DstSize. Is this a typo?

[paquette]

llvm/test/CodeGen/AArch64/GlobalISel/opt-fold-ext-tbz-tbnz.mir

80 ↗

(On Diff #255249)

I don't think this COPY is legal. This is a cross-bank copy from a 16-bit FPR onto a 32-bit GPR.

I think that we need a SUBREG_TO_REG here.

[tambre]

Generalized SUBREG_TO_REG logic

Tests now pass.

[tambre]

Fix formatting

[tambre]

Added proper generalization for the DstSize > SrcSize aka SUBREG_TO_REG case. Also moved the missing comment to SrcSize > DstSize.
Tests now pass.

llvm/lib/Target/AArch64/AArch64InstructionSelector.cpp

794–795

The above is for cases where we can't perform a subregister copy from the source bank, e.g. 32-bit to 8-bit FPR.
This handles all other subregister copies.
And I've now added a third case for promoting copies (DstSize > SrcSize) as the generalization for the 16-bit FPR to 32-bit GPR case.

llvm/test/CodeGen/AArch64/GlobalISel/opt-fold-ext-tbz-tbnz.mir

80 ↗

(On Diff #255249)

You're correct. Fixed.

[tambre]

paquette: ping

[paquette]

Where did the testcase go?

[tambre]

In D77530#1984907, @paquette wrote:

Where did the testcase go?

I didn't add any testcases. The testcase changes are now gone that I fixed the code to work correctly. This is now a NFC.

[tambre]

paquette: ping

[paquette]

LGTM!

[tambre]

Please land this for me as I lack commit privileges.

llvm/lib/Target/AArch64/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,
 
   return {getMinClassForRegBank(SrcRegBank, SrcSize, true),
           getMinClassForRegBank(DstRegBank, DstSize, 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);
 
   // Find the correct register classes for the source and destination registers.
   const TargetRegisterClass *SrcRC;
   const TargetRegisterClass *DstRC;
static bool selectCopy(MachineInstr &I, const TargetInstrInfo &TII,
   // returning true, we return isValidCopy() to ensure that we verify the
   // result.
   auto CheckCopy = [&]() {
     // If we have a bitcast or something, we can't have physical registers.
     assert((I.isCopy() ||
             (!Register::isPhysicalRegister(I.getOperand(0).getReg()) &&
              !Register::isPhysicalRegister(I.getOperand(1).getReg()))) &&
            "No phys reg on generic operator!");
-    assert(KnownValid || isValidCopy(I, DstRegBank, MRI, TRI, RBI));
-    (void)KnownValid;
-    return true;
+    bool ValidCopy = KnownValid || isValidCopy(I, DstRegBank, MRI, TRI, RBI);
+    assert(ValidCopy && "Invalid copy.");
+    return ValidCopy;
   };
 
-  // Is this a copy? If so, then we may need to insert a subregister copy, or
-  // a SUBREG_TO_REG.
+  // Is this a copy? If so, then we may need to insert a subregister copy.
   if (I.isCopy()) {
     // Yes. Check if there's anything to fix up.
     if (!SrcRC) {
       LLVM_DEBUG(dbgs() << "Couldn't determine source register class\n");
       return false;
     }
 
     unsigned SrcSize = TRI.getRegSizeInBits(*SrcRC);
     unsigned DstSize = TRI.getRegSizeInBits(*DstRC);
-
-    // If the source register is bigger than the destination we need to perform
-    // a subregister copy.
-    if (SrcSize > DstSize) {
-      unsigned SubReg = 0;
+    unsigned SubReg;
 
-      // If the source bank doesn't support a subregister copy small enough,
-      // then we first need to copy to the destination bank.
-      if (getMinSizeForRegBank(SrcRegBank) > DstSize) {
-        const TargetRegisterClass *SubregRC = getMinClassForRegBank(
-            DstRegBank, SrcSize, /* GetAllRegSet = */ true);
-        getSubRegForClass(DstRC, TRI, SubReg);
+      const TargetRegisterClass *DstTempRC =
+          getMinClassForRegBank(DstRegBank, SrcSize, /* GetAllRegSet */ true);
 
-        MachineIRBuilder MIB(I);
-        auto Copy = MIB.buildCopy({SubregRC}, {SrcReg});
-        copySubReg(I, MRI, RBI, Copy.getReg(0), DstRC, SubReg);
-      } else {
-        const TargetRegisterClass *SubregRC = getMinClassForRegBank(
-            SrcRegBank, DstSize, /* GetAllRegSet = */ true);
-        getSubRegForClass(SubregRC, TRI, SubReg);
-        copySubReg(I, MRI, RBI, SrcReg, DstRC, SubReg);
-      }
+      auto Copy = MIB.buildCopy({DstTempRC}, {SrcReg});
+    // If the source register is bigger than the destination we need to perform
+    // a subregister copy.
+    } else if (SrcSize > DstSize) {
+      const TargetRegisterClass *SubRegRC =
+          getMinClassForRegBank(SrcRegBank, DstSize, /* GetAllRegSet */ true);
+      getSubRegForClass(SubRegRC, TRI, SubReg);
+    // If the destination register is bigger than the source we need to do
+    // a promotion using SUBREG_TO_REG.
+    } else if (DstSize > SrcSize) {
+      const TargetRegisterClass *PromotionRC =
+          getMinClassForRegBank(SrcRegBank, DstSize, /* GetAllRegSet */ true);
+      getSubRegForClass(SrcRC, TRI, SubReg);
 
-      return CheckCopy();
-    }
-
-    // Is this a cross-bank copy?
-    if (DstRegBank.getID() != SrcRegBank.getID()) {
-      if (DstRegBank.getID() == AArch64::GPRRegBankID && DstSize == 32 &&
-          SrcSize == 16) {
-        // Special case for FPR16 to GPR32.
-        // FIXME: This can probably be generalized like the above case.
-        Register PromoteReg =
-            MRI.createVirtualRegister(&AArch64::FPR32RegClass);
-        BuildMI(*I.getParent(), I, I.getDebugLoc(),
-                TII.get(AArch64::SUBREG_TO_REG), PromoteReg)
-            .addImm(0)
-            .addUse(SrcReg)
-            .addImm(AArch64::hsub);
-        MachineOperand &RegOp = I.getOperand(1);
-        RegOp.setReg(PromoteReg);
+      Register PromoteReg = MRI.createVirtualRegister(PromotionRC);
+          .addImm(SubReg);
 
-        // Promise that the copy is implicitly validated by the SUBREG_TO_REG.
-        KnownValid = true;
-      }
-    }
 
     // If the destination is a physical register, then there's nothing to
     // change, so we're done.
     if (Register::isPhysicalRegister(DstReg))
       return CheckCopy();
   }
 
   // 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, *DstRC, MRI)) {