[ARM] Fix PR36249: add expansion rule for VLD1d64 pseudo with reg WB

[thopre]

Summary

VLD1d64TPseudoWB_register and VLD1d64QPseudoWB_register are missing rules to expand them to non pseudo MIR. These are selected for ARMISD::VLD3_UPD/VLD4_UPD with v1i64 vectors in SelectVLD. This patch adds the necessary mapping to VLD1d64Twb_register and VLD1d64Qwb_register respectively. As for the _fixed variant, the cost of these is bumped for unaligned access.

[thopre]

Hi all,

Ping? Best regards,

Thomas

[srhines]

This is blocking https://reviews.llvm.org/D42970, which we would also like to see fixed. This patch seems fairly straightforward to me, although I don't want to approve an ARM backend change.

[samparker]

Hi Thomas,

Please add some checks into the test case.

[thopre]

Hi Sam,

Code generation for these pseudo is already checked by the follow-on patch in https://reviews.llvm.org/D42970 (the very testcase that triggered the need for this present fix). I feel that adding checks here again would mean more testcases need to be updated in case code generation for these pseudo instructions needs to be changed. I also feel it brings a nice separation of concern: one test checks code generation, one checks correct expansion / absence of assert failure.

If you disagree let me know and I'll add the check right away.

[samparker]

If these two patches are solving the same issue, then they should be submitted for review as a single patch. As I understand, you could do that and not include this mir test and still have the functionality fully tested? This would make less work for you and the reviewer :)
Otherwise, I would say that any codegen tests should always have a test for what code is being generated, because its easy to add a check. If a test is added purely to trigger an assert or timeout, it is usually explicitly stated in the test case.

[thopre]

This has been merged into https://reviews.llvm.org/D42970, abandoning this patch.

lib/Target/ARM/ARMBaseInstrInfo.cpp

if (DefAlign < 8 && Subtarget.checkVLDnAccessAlignment())
     case ARM::VLD2q8PseudoWB_register:
     case ARM::VLD2q16PseudoWB_register:
     case ARM::VLD2q32PseudoWB_register:
     case ARM::VLD3d8Pseudo:
     case ARM::VLD3d16Pseudo:
     case ARM::VLD3d32Pseudo:
     case ARM::VLD1d64TPseudo:
     case ARM::VLD1d64TPseudoWB_fixed:
+    case ARM::VLD1d64TPseudoWB_register:
     case ARM::VLD3d8Pseudo_UPD:
     case ARM::VLD3d16Pseudo_UPD:
     case ARM::VLD3d32Pseudo_UPD:
     case ARM::VLD3q8Pseudo_UPD:
     case ARM::VLD3q16Pseudo_UPD:
     case ARM::VLD3q32Pseudo_UPD:
     case ARM::VLD3q8oddPseudo:
     case ARM::VLD3q16oddPseudo:
     case ARM::VLD3q32oddPseudo:
     case ARM::VLD3q8oddPseudo_UPD:
     case ARM::VLD3q16oddPseudo_UPD:
     case ARM::VLD3q32oddPseudo_UPD:
     case ARM::VLD4d8Pseudo:
     case ARM::VLD4d16Pseudo:
     case ARM::VLD4d32Pseudo:
     case ARM::VLD1d64QPseudo:
     case ARM::VLD1d64QPseudoWB_fixed:
+    case ARM::VLD1d64QPseudoWB_register:
     case ARM::VLD4d8Pseudo_UPD:
     case ARM::VLD4d16Pseudo_UPD:
     case ARM::VLD4d32Pseudo_UPD:
     case ARM::VLD4q8Pseudo_UPD:
     case ARM::VLD4q16Pseudo_UPD:
     case ARM::VLD4q32Pseudo_UPD:
     case ARM::VLD4q8oddPseudo:
     case ARM::VLD4q16oddPseudo:

lib/Target/ARM/ARMExpandPseudoInsts.cpp

 { ARM::VLD1LNq16Pseudo_UPD, ARM::VLD1LNd16_UPD, true, true, true,  EvenDblSpc, 1, 4 ,true},
 { ARM::VLD1LNq32Pseudo,     ARM::VLD1LNd32,     true, false, false, EvenDblSpc, 1, 2 ,true},
 { ARM::VLD1LNq32Pseudo_UPD, ARM::VLD1LNd32_UPD, true, true, true,  EvenDblSpc, 1, 2 ,true},
 { ARM::VLD1LNq8Pseudo,      ARM::VLD1LNd8,      true, false, false, EvenDblSpc, 1, 8 ,true},
 { ARM::VLD1LNq8Pseudo_UPD,  ARM::VLD1LNd8_UPD, true, true, true,  EvenDblSpc, 1, 8 ,true},
 
 { ARM::VLD1d64QPseudo,      ARM::VLD1d64Q,     true,  false, false, SingleSpc,  4, 1 ,false},
 { ARM::VLD1d64QPseudoWB_fixed,  ARM::VLD1d64Qwb_fixed,   true,  true, false, SingleSpc,  4, 1 ,false},
+{ ARM::VLD1d64QPseudoWB_register,  ARM::VLD1d64Qwb_register,   true,  true, true, SingleSpc,  4, 1 ,false},
 { ARM::VLD1d64TPseudo,      ARM::VLD1d64T,     true,  false, false, SingleSpc,  3, 1 ,false},
 { ARM::VLD1d64TPseudoWB_fixed,  ARM::VLD1d64Twb_fixed,   true,  true, false, SingleSpc,  3, 1 ,false},
+{ ARM::VLD1d64TPseudoWB_register,  ARM::VLD1d64Twb_register, true, true, true,  SingleSpc,  3, 1 ,false},
 
 { ARM::VLD2LNd16Pseudo,     ARM::VLD2LNd16,     true, false, false, SingleSpc,  2, 4 ,true},
 { ARM::VLD2LNd16Pseudo_UPD, ARM::VLD2LNd16_UPD, true, true, true,  SingleSpc,  2, 4 ,true},
 { ARM::VLD2LNd32Pseudo,     ARM::VLD2LNd32,     true, false, false, SingleSpc,  2, 2 ,true},
 { ARM::VLD2LNd32Pseudo_UPD, ARM::VLD2LNd32_UPD, true, true, true,  SingleSpc,  2, 2 ,true},
 { ARM::VLD2LNd8Pseudo,      ARM::VLD2LNd8,      true, false, false, SingleSpc,  2, 8 ,true},
 { ARM::VLD2LNd8Pseudo_UPD,  ARM::VLD2LNd8_UPD, true, true, true,  SingleSpc,  2, 8 ,true},
 { ARM::VLD2LNq16Pseudo,     ARM::VLD2LNq16,     true, false, false, EvenDblSpc, 2, 4 ,true},
switch (Opcode) {
     case ARM::VLD2q8PseudoWB_register:
     case ARM::VLD2q16PseudoWB_register:
     case ARM::VLD2q32PseudoWB_register:
     case ARM::VLD3d8Pseudo:
     case ARM::VLD3d16Pseudo:
     case ARM::VLD3d32Pseudo:
     case ARM::VLD1d64TPseudo:
     case ARM::VLD1d64TPseudoWB_fixed:
+    case ARM::VLD1d64TPseudoWB_register:
     case ARM::VLD3d8Pseudo_UPD:
     case ARM::VLD3d16Pseudo_UPD:
     case ARM::VLD3d32Pseudo_UPD:
     case ARM::VLD3q8Pseudo_UPD:
     case ARM::VLD3q16Pseudo_UPD:
     case ARM::VLD3q32Pseudo_UPD:
     case ARM::VLD3q8oddPseudo:
     case ARM::VLD3q16oddPseudo:
     case ARM::VLD3q32oddPseudo:
     case ARM::VLD3q8oddPseudo_UPD:
     case ARM::VLD3q16oddPseudo_UPD:
     case ARM::VLD3q32oddPseudo_UPD:
     case ARM::VLD4d8Pseudo:
     case ARM::VLD4d16Pseudo:
     case ARM::VLD4d32Pseudo:
     case ARM::VLD1d64QPseudo:
     case ARM::VLD1d64QPseudoWB_fixed:
+    case ARM::VLD1d64QPseudoWB_register:
     case ARM::VLD4d8Pseudo_UPD:
     case ARM::VLD4d16Pseudo_UPD:
     case ARM::VLD4d32Pseudo_UPD:
     case ARM::VLD4q8Pseudo_UPD:
     case ARM::VLD4q16Pseudo_UPD:
     case ARM::VLD4q32Pseudo_UPD:
     case ARM::VLD4q8oddPseudo:
     case ARM::VLD4q16oddPseudo:

test/CodeGen/ARM/pr36249.mir

+# RUN: llc -mtriple armv7 %s -o -
+
+name:            vld3i64
+tracksRegLiveness: true
+registers:
+  - { id: 0, class: gpr }
+  - { id: 1, class: gpr }
+  - { id: 2, class: rgpr }
+  - { id: 3, class: qqpr }
+  - { id: 4, class: gpr }
+  - { id: 5, class: dpr }
+  - { id: 6, class: gpr }
+  - { id: 7, class: gpr }
+frameInfo:
+  maxCallFrameSize: 0
+body:             |
+  bb.0:
+    liveins: %r0, %r1
+
+    %1:gpr = COPY %r1
+    %0:gpr = COPY %r0
+    %2:rgpr = MOVi 4, 14, %noreg, %noreg
+    %3:qqpr, %4:gpr = VLD1d64TPseudoWB_register %0, 8, killed %2, 14, %noreg :: (load 24, align 8)
+    %5:dpr = COPY %3.dsub_0
+    %6:gpr, %7:gpr = VMOVRRD killed %5, 14, %noreg
+    STRi12 killed %4, %1, 0, 14, %noreg ::
+    %r0 = COPY %6
+    %r1 = COPY %7
+    BX_RET 14, %noreg, implicit %r0, implicit %r1
+
+...
+---
+name:            vld4i64
+tracksRegLiveness: true
+registers:
+  - { id: 0, class: gpr }
+  - { id: 1, class: gpr }
+  - { id: 2, class: rgpr }
+  - { id: 3, class: qqpr }
+  - { id: 4, class: gpr }
+  - { id: 5, class: dpr }
+  - { id: 6, class: gpr }
+  - { id: 7, class: gpr }
+frameInfo:
+  maxCallFrameSize: 0
+body:             |
+  bb.0:
+    liveins: %r0, %r1
+
+    %1:gpr = COPY %r1
+    %0:gpr = COPY %r0
+    %2:rgpr = MOVi 4, 14, %noreg, %noreg
+    %3:qqpr, %4:gpr = VLD1d64QPseudoWB_register %0, 8, killed %2, 14, %noreg :: (load 32, align 8)
+    %5:dpr = COPY %3.dsub_0
+    %6:gpr, %7:gpr = VMOVRRD killed %5, 14, %noreg
+    STRi12 killed %4, %1, 0, 14, %noreg ::
+    %r0 = COPY %6
+    %r1 = COPY %7
+    BX_RET 14, %noreg, implicit %r0, implicit %r1
+