[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 ↗

(On Diff #407598)

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

     return all_of(RequiredClasses, [PR, TRI](const TargetRegisterClass *C) {
       return any_of(TRI->sub_and_superregs_inclusive(PR),
                     [C, TRI](MCPhysReg SubOrSuper) {
                       return C == TRI->getMinimalPhysRegClass(SubOrSuper);
                     });
     });
   };
 
+  // Checks if any instr in between FirstMI and MI has renamable undef that
+  // overlap with PR.
+  auto UsedByUndefInBetween = [TRI](MachineInstr &FirstMI, MachineInstr &MI,
+                                    const MCPhysReg &PR) {
+    for (auto &MI :
+         make_range(std::next(FirstMI.getIterator()), MI.getIterator())) {
+      for (auto &MOP : MI.operands()) {
+        if (MOP.isReg() && MOP.isUndef() && MOP.isRenamable() &&
+            TRI->regsOverlap(MOP.getReg(), PR))
+          return true;
+      }
+    }
+    return false;
+  };
+
   auto *RegClass = TRI->getMinimalPhysRegClass(getLdStRegOp(FirstMI).getReg());
   for (const MCPhysReg &PR : *RegClass) {
     if (DefinedInBB.available(PR) && UsedInBetween.available(PR) &&
         !RegInfo.isReserved(PR) && !AnySubOrSuperRegCalleePreserved(PR) &&
-        CanBeUsedForAllClasses(PR)) {
+        CanBeUsedForAllClasses(PR) && !UsedByUndefInBetween(FirstMI, MI, PR)) {
       DefinedInBB.addReg(PR);
       LLVM_DEBUG(dbgs() << "Found rename register " << printReg(PR, TRI)
                         << "\n");
       return {PR};
     }
   }
   LLVM_DEBUG(dbgs() << "No rename register found from "
                     << TRI->getRegClassName(RegClass) << "\n");

llvm/test/CodeGen/AArch64/stp-opt-with-renaming-crash.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 crashing with:
+# "Rename register used between paired instruction, trashing the content".
+# The crash was caused by picking renamable register q16, which is not used
+# as defs, not liveins, not reserved or callee preserved and can be used for
+# all class. However overlap with renamable undef d16 used by ZIP2 instruction.
+
+# 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: $q17 = 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 $q17, $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
+...