[CodeGen][ARM] Test validity of SubRC + SubReg in shouldRewriteCopySrc

[sdesmalen-arm]

For the ARM target, registers S0..S31 overlap with D0..D15. Subreg indices ssub_0 and ssub_1 are only valid for a register class that covers D0..D15, but the register class passed to shouldRewriteCopySrc is DPRRegClass which covers D0..D15 as well as D16..D31, meaning that ssub_0 and ssub_1 can't be used.

By fixing this in TargetRegisterInfo::shouldRewriteCopySrc, there's no longer a need to override shouldRewriteCopySrc for ARM.

This fixes #159343

[llvmbot]

@llvm/pr-subscribers-backend-arm

Author: Sander de Smalen (sdesmalen-arm)

Changes

For the ARM target, registers S0..S31 overlap with D0..D15. Subreg indices ssub_0 and ssub_1 are only valid for a register class that covers D0..D15, but the register class passed to shouldRewriteCopySrc is DPRRegClass which covers D0..D15 as well as D16..D31, meaning that ssub_0 and ssub_1 can't be used.

By fixing this in TargetRegisterInfo::shouldRewriteCopySrc, there's no longer a need to override shouldRewriteCopySrc for ARM.

This fixes #159343

---

Full diff: https://github.com/llvm/llvm-project/pull/159600.diff

4 Files Affected:

• (modified) llvm/include/llvm/CodeGen/TargetRegisterInfo.h (+5)
• (modified) llvm/lib/Target/ARM/ARMBaseRegisterInfo.cpp (-14)
• (modified) llvm/lib/Target/ARM/ARMBaseRegisterInfo.h (-5)
• (added) llvm/test/CodeGen/ARM/pr159343.mir (+31)

diff --git a/llvm/include/llvm/CodeGen/TargetRegisterInfo.h b/llvm/include/llvm/CodeGen/TargetRegisterInfo.h
index bf133f0332bcb..8d126af67f2a7 100644
--- a/llvm/include/llvm/CodeGen/TargetRegisterInfo.h
+++ b/llvm/include/llvm/CodeGen/TargetRegisterInfo.h
@@ -700,6 +700,11 @@ class LLVM_ABI TargetRegisterInfo : public MCRegisterInfo {
                                     unsigned DefSubReg,
                                     const TargetRegisterClass *SrcRC,
                                     unsigned SrcSubReg) const {
+    // Validate that SrcRC and SrcSubReg is actually a valid combination.
+    const TargetRegisterClass *SubRC = getSubClassWithSubReg(SrcRC, SrcSubReg);
+    if (!SubRC || SubRC != SrcRC)
+      return false;
+
     // If this source does not incur a cross register bank copy, use it.
     return findCommonRegClass(DefRC, DefSubReg, SrcRC, SrcSubReg) != nullptr;
   }
diff --git a/llvm/lib/Target/ARM/ARMBaseRegisterInfo.cpp b/llvm/lib/Target/ARM/ARMBaseRegisterInfo.cpp
index e94220af05a0d..2e8a676269a74 100644
--- a/llvm/lib/Target/ARM/ARMBaseRegisterInfo.cpp
+++ b/llvm/lib/Target/ARM/ARMBaseRegisterInfo.cpp
@@ -960,17 +960,3 @@ bool ARMBaseRegisterInfo::shouldCoalesce(MachineInstr *MI,
   }
   return false;
 }
-
-bool ARMBaseRegisterInfo::shouldRewriteCopySrc(const TargetRegisterClass *DefRC,
-                                               unsigned DefSubReg,
-                                               const TargetRegisterClass *SrcRC,
-                                               unsigned SrcSubReg) const {
-  // We can't extract an SPR from an arbitary DPR (as opposed to a DPR_VFP2).
-  if (DefRC == &ARM::SPRRegClass && DefSubReg == 0 &&
-      SrcRC == &ARM::DPRRegClass &&
-      (SrcSubReg == ARM::ssub_0 || SrcSubReg == ARM::ssub_1))
-    return false;
-
-  return TargetRegisterInfo::shouldRewriteCopySrc(DefRC, DefSubReg,
-                                                  SrcRC, SrcSubReg);
-}
diff --git a/llvm/lib/Target/ARM/ARMBaseRegisterInfo.h b/llvm/lib/Target/ARM/ARMBaseRegisterInfo.h
index 5b67b34089d7e..03b0fa0d1ee08 100644
--- a/llvm/lib/Target/ARM/ARMBaseRegisterInfo.h
+++ b/llvm/lib/Target/ARM/ARMBaseRegisterInfo.h
@@ -158,11 +158,6 @@ class ARMBaseRegisterInfo : public ARMGenRegisterInfo {
                       const TargetRegisterClass *NewRC,
                       LiveIntervals &LIS) const override;
 
-  bool shouldRewriteCopySrc(const TargetRegisterClass *DefRC,
-                            unsigned DefSubReg,
-                            const TargetRegisterClass *SrcRC,
-                            unsigned SrcSubReg) const override;
-
   int getSEHRegNum(unsigned i) const { return getEncodingValue(i); }
 };
 
diff --git a/llvm/test/CodeGen/ARM/pr159343.mir b/llvm/test/CodeGen/ARM/pr159343.mir
new file mode 100644
index 0000000000000..f9ad8af2cfff6
--- /dev/null
+++ b/llvm/test/CodeGen/ARM/pr159343.mir
@@ -0,0 +1,31 @@
+# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py UTC_ARGS: --version 6
+# RUN: llc -run-pass=peephole-opt -verify-machineinstrs -mtriple=thumbv7-unknown-linux-android29 %s -o - | FileCheck %s
+---
+name: Test_shouldRewriteCopySrc_Invalid_SubReg
+tracksRegLiveness: true
+body:             |
+  bb.1:
+    liveins: $r0, $r1
+
+    ; CHECK-LABEL: name: Test_shouldRewriteCopySrc_Invalid_SubReg
+    ; CHECK: liveins: $r0, $r1
+    ; CHECK-NEXT: {{  $}}
+    ; CHECK-NEXT: [[DEF:%[0-9]+]]:dpair = IMPLICIT_DEF
+    ; CHECK-NEXT: [[COPY:%[0-9]+]]:dpr = COPY [[DEF]].dsub_0
+    ; CHECK-NEXT: [[VMOVRRD:%[0-9]+]]:gpr, [[VMOVRRD1:%[0-9]+]]:gpr = VMOVRRD killed [[COPY]], 14 /* CC::al */, $noreg
+    ; CHECK-NEXT: [[VMOVSR:%[0-9]+]]:spr = VMOVSR killed [[VMOVRRD1]], 14 /* CC::al */, $noreg
+    ; CHECK-NEXT: [[DEF1:%[0-9]+]]:spr = IMPLICIT_DEF
+    ; CHECK-NEXT: [[DEF2:%[0-9]+]]:spr = IMPLICIT_DEF
+    ; CHECK-NEXT: [[DEF3:%[0-9]+]]:spr = IMPLICIT_DEF
+    ; CHECK-NEXT: [[REG_SEQUENCE:%[0-9]+]]:mqpr = REG_SEQUENCE killed [[DEF2]], %subreg.ssub_0, killed [[DEF1]], %subreg.ssub_1, killed [[DEF3]], %subreg.ssub_2, killed [[VMOVSR]], %subreg.ssub_3
+    ; CHECK-NEXT: VST1q64 $r1, 0, killed [[REG_SEQUENCE]], 14 /* CC::al */, $noreg
+    %0:dpair = IMPLICIT_DEF
+    %1:dpr = COPY %0.dsub_0
+    %2:gpr, %3:gpr = VMOVRRD killed %1, 14 /* CC::al */, $noreg
+    %4:spr = VMOVSR killed %3, 14 /* CC::al */, $noreg
+    %5:spr = IMPLICIT_DEF
+    %6:spr = IMPLICIT_DEF
+    %7:spr = IMPLICIT_DEF
+    %8:mqpr = REG_SEQUENCE killed %6, %subreg.ssub_0, killed %5, %subreg.ssub_1, killed %7, %subreg.ssub_2, killed %4, %subreg.ssub_3
+    VST1q64 $r1, 0, killed %8, 14 /* CC::al */, $noreg
+...

[arsenm]

Looking at this again, this does feel like the wrong place to handle this. This should either be sunk into findCommonRegClass, or pulled out into the caller. Did you find the place that originated the SrcRC + SubReg combination? Did it happen to be from the UncoalescableRewriter?

[arsenm]

I ran into this issue again, and I don't understand what ARM is doing. getExtractSubregInputs is returning a register + subregister pair, where the class of the register doesn't support that subregister index. I suppose that sort of makes sense, but it requires re-constraining the register if it actually folds into a use (but that's not done currently)

[arsenm]

I think I have an alternative patch for this

[sdesmalen-arm]

Closing in favour of #161338