[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

764–765

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

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

764–765

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

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;
   std::tie(SrcRC, DstRC) = getRegClassesForCopy(I, TII, MRI, TRI, RBI);
 
   if (!DstRC) {
     LLVM_DEBUG(dbgs() << "Unexpected dest size "
                       << RBI.getSizeInBits(DstReg, MRI, TRI) << '\n');
     return false;
   }
 
-  // A couple helpers below, for making sure that the copy we produce is valid.
-
-  // Set to true if we insert a SUBREG_TO_REG. If we do this, then we don't want
-  // to verify that the src and dst are the same size, since that's handled by
-  // the SUBREG_TO_REG.
-  bool KnownValid = false;
-
   // Returns true, or asserts if something we don't expect happens. Instead of
   // 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 = 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;
 
-      // 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);
+          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);
-      }
+    } else if (SrcSize > DstSize) {
+          SrcRegBank, DstSize, /* GetAllRegSet */ true);
 
-      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);
-
-        // 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.

llvm/test/CodeGen/AArch64/GlobalISel/fp16-copy-gpr.mir

body:             |
   bb.1 (%ir-block.1):
     liveins: $h0, $h1, $x0
 
     ; CHECK-LABEL: name: fp16_to_gpr
     ; CHECK: liveins: $h0, $h1, $x0
     ; CHECK: [[COPY:%[0-9]+]]:fpr16 = COPY $h0
     ; CHECK: [[COPY1:%[0-9]+]]:fpr16 = COPY $h1
     ; CHECK: [[DEF:%[0-9]+]]:gpr32 = IMPLICIT_DEF
-    ; CHECK: [[SUBREG_TO_REG:%[0-9]+]]:fpr32 = SUBREG_TO_REG 0, [[COPY]], %subreg.hsub
-    ; CHECK: [[COPY2:%[0-9]+]]:gpr32 = COPY [[SUBREG_TO_REG]]
+    ; CHECK: [[COPY2:%[0-9]+]]:gpr32 = COPY [[COPY]]
     ; CHECK: [[BFMWri:%[0-9]+]]:gpr32 = BFMWri [[DEF]], [[COPY2]], 0, 15
-    ; CHECK: [[SUBREG_TO_REG1:%[0-9]+]]:fpr32 = SUBREG_TO_REG 0, [[COPY1]], %subreg.hsub
-    ; CHECK: [[COPY3:%[0-9]+]]:gpr32 = COPY [[SUBREG_TO_REG1]]
+    ; CHECK: [[COPY3:%[0-9]+]]:gpr32 = COPY [[COPY1]]
     ; CHECK: [[BFMWri1:%[0-9]+]]:gpr32 = BFMWri [[BFMWri]], [[COPY3]], 16, 15
     ; CHECK: [[COPY4:%[0-9]+]]:gpr64sp = COPY $x0
     ; CHECK: STRWui [[BFMWri1]], [[COPY4]], 0 :: (store 4 into %ir.addr, align 2)
     ; CHECK: RET_ReallyLR
     %1:fpr(s16) = COPY $h0
     %2:fpr(s16) = COPY $h1
     %3:gpr(s32) = G_IMPLICIT_DEF
     %11:gpr(s16) = COPY %1(s16)

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

 legalized:       true
 regBankSelected: true
 tracksRegLiveness: true
 body:             |
   ; CHECK-LABEL: name: fold_multiple
   ; CHECK: bb.0:
   ; CHECK:   successors: %bb.0(0x40000000), %bb.1(0x40000000)
   ; CHECK:   liveins: $h0
-  ; CHECK:   [[SUBREG_TO_REG:%[0-9]+]]:fpr32 = SUBREG_TO_REG 0, $h0, %subreg.hsub
-  ; CHECK:   %copy:gpr32 = COPY [[SUBREG_TO_REG]]
+  ; CHECK:   %copy:gpr32 = COPY $h0
   ; CHECK:   TBNZW %copy, 3, %bb.1
   ; CHECK:   B %bb.0
   ; CHECK: bb.1:
   ; CHECK:   RET_ReallyLR
   bb.0:
     successors: %bb.0, %bb.1
     liveins: $h0
     %copy:gpr(s16) = COPY $h0

llvm/test/CodeGen/AArch64/GlobalISel/preselect-process-phis.mir

body:             |
   ; CHECK:   successors: %bb.2(0x80000000)
   ; CHECK:   [[DEF2:%[0-9]+]]:gpr32 = IMPLICIT_DEF
   ; CHECK:   $wzr = ANDSWri [[DEF]], 0, implicit-def $nzcv
   ; CHECK:   [[CSELWr:%[0-9]+]]:gpr32 = CSELWr [[DEF2]], [[DEF2]], 1, implicit $nzcv
   ; CHECK: bb.2:
   ; CHECK:   successors: %bb.2(0x80000000)
   ; CHECK:   [[PHI:%[0-9]+]]:gpr32 = PHI [[CSELWr]], %bb.1, %8, %bb.2
   ; CHECK:   [[FCVTHSr:%[0-9]+]]:fpr16 = FCVTHSr [[COPY]]
-  ; CHECK:   [[SUBREG_TO_REG:%[0-9]+]]:fpr32 = SUBREG_TO_REG 0, [[FCVTHSr]], %subreg.hsub
-  ; CHECK:   [[COPY1:%[0-9]+]]:gpr32all = COPY [[SUBREG_TO_REG]]
+  ; CHECK:   [[COPY1:%[0-9]+]]:gpr32all = COPY [[FCVTHSr]]
   ; CHECK:   STRHHui [[PHI]], [[DEF1]], 0 :: (store 2 into `half* undef`)
   ; CHECK:   B %bb.2
   bb.0:
     successors: %bb.1(0x80000000)
 
     %0:gpr(s1) = G_IMPLICIT_DEF
     %4:gpr(p0) = G_IMPLICIT_DEF
     %8:fpr(s32) = G_FCONSTANT float 0x7FF8000000000000