[AArch64][LoadStoreOptimizer] Ignore undef registers when checking rename register used between paired instructions.

[huihuiz]

Summary

The content of undef registers are not used in meaningful ways, when checking
if a rename register is used between paired instructions we should ignore
undef registers.

[huihuiz]

Take test mir attached test/CodeGen/AArch64/stp-opt-with-renaming-crash.mir
run with llc -run-pass=aarch64-ldst-opt -mtriple=aarch64 -verify-machineinstrs

Then you will see assertion
"Rename register used between paired instruction, trashing the content".

Although we don't really care trashing content of an undef, but should we exclude undef when checking for overlap ?

#if !defined(NDEBUG)
    // Make sure the register used for renaming is not used between the paired
    // instructions. That would trash the content before the new paired
    // instruction.
    for (auto &MI :
         iterator_range<MachineInstrBundleIterator<llvm::MachineInstr>>(
             std::next(I), std::next(Paired)))
      assert(all_of(MI.operands(),
                    [this, &RenameReg](const MachineOperand &MOP) {
                      return !MOP.isReg() || MOP.isDebug() || !MOP.getReg() ||
                             **MOP.isUndef() ||**
                             !TRI->regsOverlap(MOP.getReg(), *RenameReg);
                    }) &&
             "Rename register used between paired instruction, trashing the "
             "content");
#endif

[efriedma]

Probably the code printing the "Rename register used between paired instruction" diagnostic should ignore undef use; such uses don't use the value in any meaningful way.

For reference, the code in LiveRegUnits considers isDef() a write, and readsReg() a read.

[huihuiz]

Thanks Eli for the feedbacks!
Exclude undef from overlap diagnosis.

[efriedma]

LGTM

[fhahn]

Thanks for fixing that!

llvm/test/CodeGen/AArch64/stp-opt-with-renaming-undef-assert.mir

40

Are those needed or could be dropped?

[huihuiz]

Thanks @fhahn , stack accesses are not needed. I just clean them in https://reviews.llvm.org/rG19302cd7a449

llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp

#if !defined(NDEBUG)
     // instructions. That would trash the content before the new paired
     // instruction.
     for (auto &MI :
          iterator_range<MachineInstrBundleIterator<llvm::MachineInstr>>(
              std::next(I), std::next(Paired)))
       assert(all_of(MI.operands(),
                     [this, &RenameReg](const MachineOperand &MOP) {
                       return !MOP.isReg() || MOP.isDebug() || !MOP.getReg() ||
+                             MOP.isUndef() ||
                              !TRI->regsOverlap(MOP.getReg(), *RenameReg);
                     }) &&
              "Rename register used between paired instruction, trashing the "
              "content");
 #endif
   }
 
   // Insert our new paired instruction after whichever of the paired

llvm/test/CodeGen/AArch64/stp-opt-with-renaming-undef-assert.mir

+# RUN: llc -run-pass=aarch64-ldst-opt -mtriple=aarch64 -verify-machineinstrs -o - %s | FileCheck %s
+
+# This test checks that aarch64 load store optimizer is not throwing an
+# assertion:
+# "Rename register used between paired instruction, trashing the content".
+#
+# The assertion was previously triggered because q16 is picked as renamable
+# register, which overlap with renamable undef d16 used by ZIP2 instruction.
+# However, the content of an undef register is not used in meaningful way,
+# aarch64 load store optimizer should not throw an assertion if a renamable
+# register picked overlap with a renamable undef register.
+
+# This test also checks that pairwise store STP is generated.
+
+# CHECK-LABLE: test
+# CHECK: bb.0:
+# CHECK-NEXT: liveins: $x0, $x17, $x18
+# CHECK: renamable $q13_q14_q15 = LD3Threev16b undef renamable $x17 :: (load (s384) from `<16 x i8>* undef`, align 64)
+# CHECK-NEXT: renamable $q23_q24_q25 = LD3Threev16b undef renamable $x18 :: (load (s384) from `<16 x i8>* undef`, align 64)
+# CHECK-NEXT: $q16 = EXTv16i8 renamable $q23, renamable $q23, 8
+# CHECK-NEXT: renamable $q20 = EXTv16i8 renamable $q14, renamable $q14, 8
+# CHECK-NEXT: STRQui killed renamable $q20, $sp, 4 :: (store (s128) into %stack.3)
+# CHECK-NEXT: renamable $d6 = ZIP2v8i8 renamable $d23, undef renamable $d16
+# CHECK-NEXT: STRDui killed renamable $d6, $sp, 11 :: (store (s64) into %stack.2)
+# CHECK-NEXT: renamable $q6 = EXTv16i8 renamable $q13, renamable $q13, 8
+# CHECK-NEXT: STPQi killed renamable $q6, killed $q16, $sp, 6 :: (store (s128) into %stack.0), (store (s128) into %stack.1)
+# CHECK-NEXT: RET undef $lr
+
+---
+name:            test
+alignment:       4
+tracksRegLiveness: true
+liveins:
+  - { reg: '$x0', virtual-reg: '' }
+frameInfo:
+  maxAlignment:    16
+  maxCallFrameSize: 0
+fixedStack:      []
+stack:
+  - { id: 0, name: '', type: spill-slot, offset: -80, size: 16, alignment: 16,
+      stack-id: default, callee-saved-register: '', callee-saved-restored: true,
+      debug-info-variable: '', debug-info-expression: '', debug-info-location: '' }
+  - { id: 1, name: '', type: spill-slot, offset: -96, size: 16, alignment: 16,
+      stack-id: default, callee-saved-register: '', callee-saved-restored: true,
+      debug-info-variable: '', debug-info-expression: '', debug-info-location: '' }
+  - { id: 2, name: '', type: spill-slot, offset: -104, size: 8, alignment: 8,
+      stack-id: default, callee-saved-register: '', callee-saved-restored: true,
+      debug-info-variable: '', debug-info-expression: '', debug-info-location: '' }
+  - { id: 3, name: '', type: spill-slot, offset: -128, size: 16, alignment: 16,
+      stack-id: default, callee-saved-register: '', callee-saved-restored: true,
+      debug-info-variable: '', debug-info-expression: '', debug-info-location: '' }
+machineFunctionInfo: {}
+body:             |
+  bb.0:
+    liveins: $x0, $x17, $x18
+    renamable $q13_q14_q15 = LD3Threev16b undef renamable $x17 :: (load (s384) from `<16 x i8>* undef`, align 64)
+    renamable $q23_q24_q25 = LD3Threev16b undef renamable $x18 :: (load (s384) from `<16 x i8>* undef`, align 64)
+    renamable $q20 = EXTv16i8 renamable $q23, renamable $q23, 8
+    STRQui killed renamable $q20, $sp, 7 :: (store (s128) into %stack.0)
+    renamable $q20 = EXTv16i8 renamable $q14, renamable $q14, 8
+    STRQui killed renamable $q20, $sp, 4 :: (store (s128) into %stack.3)
+    renamable $d6 = ZIP2v8i8 renamable $d23, undef renamable $d16
+    STRDui killed renamable $d6, $sp, 11 :: (store (s64) into %stack.2)
+    renamable $q6 = EXTv16i8 renamable $q13, renamable $q13, 8
+    STRQui killed renamable $q6, $sp, 6 :: (store (s128) into %stack.1)
+    RET undef $lr
+...