[PATCH] D77530: [AArch64][GlobalISel] Generalize logic for subregister copies

[Raul Tambre via Phabricator via llvm-commits]

tambre added a comment.

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



================
Comment at: llvm/lib/Target/AArch64/AArch64InstructionSelector.cpp:764-765
+      copySubReg(I, MRI, RBI, Copy.getReg(0), DstRC, SubReg);
+    } else if (SrcSize > DstSize) {
+      const TargetRegisterClass *SubregRC = getMinClassForRegBank(
+          SrcRegBank, DstSize, /* GetAllRegSet */ true);
----------------
paquette wrote:
> It's weird that we have this case and also the case above which checks `getMinSizeForRegBank(SrcRegBank) > DstSize`. Is this a typo?
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.


================
Comment at: llvm/test/CodeGen/AArch64/GlobalISel/opt-fold-ext-tbz-tbnz.mir:80
   ; 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
----------------
paquette wrote:
> 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.
You're correct. Fixed.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77530/new/

https://reviews.llvm.org/D77530