[AArch64] Load/store opt: Don't count transient instructions towards search limits.

[gberry]

Summary

This change also changes findMatchingInsn and
findMatchingUpdateInsnForward to take DBG_VALUE opcodes into account
when tracking register defs and uses, which could potentially inhibit
these optimizations in the presence of debug information.

[mcrosier]

The change itself looks fine. Would it be possible to create a stable test case?

[t.p.northover]

I was thinking the same thing. Should be relatively simple with MIR: just spam some ignorable instructions into otherwise tempting situations.

[gberry]

I'll give it a shot

[gberry]

Add MIR test cases.

[t.p.northover]

I think you also need to declare initializeAArch64LoadStoreOptPass in AArch64.h and call it in AArch64TargetMachine.cpp (otherwise llc won't recognize aarch64-ldst-opt).

Other than that, it all looks reasonable to me now.

[gberry]

Hi Tim,

I went ahead and made that change in https://reviews.llvm.org/rL276193

[t.p.northover]

Yes sorry, I did notice that afterwards but thought it would all work out without extra comments. LGTM!

lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp

bool AArch64LoadStoreOpt::findMatchingStore(
   ModifiedRegs.reset();
   UsedRegs.reset();
 
   unsigned Count = 0;
   do {
     --MBBI;
     MachineInstr &MI = *MBBI;
 
-    // Don't count DBG_VALUE instructions towards the search limit.
-    if (!MI.isDebugValue())
+    // Don't count transient instructions towards the search limit since there
+    // may be different numbers of them if e.g. debug information is present.
+    if (!MI.isTransient())
       ++Count;
 
     // If the load instruction reads directly from the address to which the
     // store instruction writes and the stored value is not modified, we can
     // promote the load. Since we do not handle stores with pre-/post-index,
     // it's unnecessary to check if BaseReg is modified by the store itself.
     if (MI.mayStore() && isMatchingStore(LoadMI, MI) &&
         BaseReg == getLdStBaseOp(MI).getReg() &&
AArch64LoadStoreOpt::findMatchingInsn(MachineBasicBlock::iterator I,
   ModifiedRegs.reset();
   UsedRegs.reset();
 
   // Remember any instructions that read/write memory between FirstMI and MI.
   SmallVector<MachineInstr *, 4> MemInsns;
 
   for (unsigned Count = 0; MBBI != E && Count < Limit; ++MBBI) {
     MachineInstr &MI = *MBBI;
-    // Skip DBG_VALUE instructions. Otherwise debug info can affect the
-    // optimization by changing how far we scan.
-    if (MI.isDebugValue())
-      continue;
 
-    // Now that we know this is a real instruction, count it.
+    // Don't count transient instructions towards the search limit since there
+    // may be different numbers of them if e.g. debug information is present.
+    if (!MI.isTransient())
       ++Count;
 
     Flags.setSExtIdx(-1);
     if (areCandidatesToMergeOrPair(FirstMI, MI, Flags, TII) &&
         getLdStOffsetOp(MI).isImm()) {
       assert(MI.mayLoadOrStore() && "Expected memory operation.");
       // If we've found another instruction with the same opcode, check to see
       // if the base and offset are compatible with our starting instruction.
       // These instructions all have scaled immediate operands, so we just
MachineBasicBlock::iterator AArch64LoadStoreOpt::findMatchingUpdateInsnForward(
 
   // Track which registers have been modified and used between the first insn
   // (inclusive) and the second insn.
   ModifiedRegs.reset();
   UsedRegs.reset();
   ++MBBI;
   for (unsigned Count = 0; MBBI != E && Count < Limit; ++MBBI) {
     MachineInstr &MI = *MBBI;
-    // Skip DBG_VALUE instructions.
-    if (MI.isDebugValue())
-      continue;
 
-    // Now that we know this is a real instruction, count it.
+    // Don't count transient instructions towards the search limit since there
+    // may be different numbers of them if e.g. debug information is present.
+    if (!MI.isTransient())
       ++Count;
 
     // If we found a match, return it.
     if (isMatchingUpdateInsn(*I, MI, BaseReg, UnscaledOffset))
       return MBBI;
 
     // Update the status of what the instruction clobbered and used.
     trackRegDefsUses(MI, ModifiedRegs, UsedRegs, TRI);
 
MachineBasicBlock::iterator AArch64LoadStoreOpt::findMatchingUpdateInsnBackward(
   // (inclusive) and the second insn.
   ModifiedRegs.reset();
   UsedRegs.reset();
   unsigned Count = 0;
   do {
     --MBBI;
     MachineInstr &MI = *MBBI;
 
-    // Don't count DBG_VALUE instructions towards the search limit.
-    if (!MI.isDebugValue())
+    // Don't count transient instructions towards the search limit since there
+    // may be different numbers of them if e.g. debug information is present.
+    if (!MI.isTransient())
       ++Count;
 
     // If we found a match, return it.
     if (isMatchingUpdateInsn(*I, MI, BaseReg, Offset))
       return MBBI;
 
     // Update the status of what the instruction clobbered and used.
     trackRegDefsUses(MI, ModifiedRegs, UsedRegs, TRI);

test/CodeGen/AArch64/ldst-opt-dbg-limit.mir

+# RUN: llc -run-pass=aarch64-ldst-opt %s -o - 2>&1 | FileCheck %s
+--- |
+  target datalayout = "e-m:e-i8:8:32-i16:16:32-i64:64-i128:128-n32:64-S128"
+  target triple = "aarch64--linux-gnu"
+
+  ; Function Attrs: nounwind
+  define i16 @promote-load-from-store(i32* %dst, i32 %x) #0 {
+    store i32 %x, i32* %dst
+    %dst16 = bitcast i32* %dst to i16*
+    %dst1 = getelementptr inbounds i16, i16* %dst16, i32 1
+    %x16 = load i16, i16* %dst1
+    ret i16 %x16
+  }
+  
+  ; Function Attrs: nounwind
+  define void @store-pair(i32* %dst, i32 %x, i32 %y) #0 {
+    %dst01 = bitcast i32* %dst to i32*
+    %dst1 = getelementptr inbounds i32, i32* %dst, i32 1
+    store i32 %x, i32* %dst01
+    store i32 %x, i32* %dst1
+    ret void
+  }
+  
+  attributes #0 = { nounwind }
+
+...
+---
+name:            promote-load-from-store
+alignment:       2
+exposesReturnsTwice: false
+hasInlineAsm:    false
+allVRegsAllocated: true
+isSSA:           false
+tracksRegLiveness: false
+tracksSubRegLiveness: false
+liveins:         
+  - { reg: '%x0' }
+  - { reg: '%w1' }
+frameInfo:       
+  isFrameAddressTaken: false
+  isReturnAddressTaken: false
+  hasStackMap:     false
+  hasPatchPoint:   false
+  stackSize:       0
+  offsetAdjustment: 0
+  maxAlignment:    0
+  adjustsStack:    false
+  hasCalls:        false
+  maxCallFrameSize: 0
+  hasOpaqueSPAdjustment: false
+  hasVAStart:      false
+  hasMustTailInVarArgFunc: false
+body:             |
+  bb.0 (%ir-block.0):
+    liveins: %w1, %x0
+  
+    STRWui killed %w1, %x0, 0 :: (store 4 into %ir.dst)
+    CFI_INSTRUCTION 0
+    CFI_INSTRUCTION 0
+    CFI_INSTRUCTION 0
+    CFI_INSTRUCTION 0
+    CFI_INSTRUCTION 0
+    CFI_INSTRUCTION 0
+    CFI_INSTRUCTION 0
+    CFI_INSTRUCTION 0
+    CFI_INSTRUCTION 0
+    CFI_INSTRUCTION 0
+    CFI_INSTRUCTION 0
+    CFI_INSTRUCTION 0
+    CFI_INSTRUCTION 0
+    CFI_INSTRUCTION 0
+    CFI_INSTRUCTION 0
+    CFI_INSTRUCTION 0
+    CFI_INSTRUCTION 0
+    CFI_INSTRUCTION 0
+    CFI_INSTRUCTION 0
+    CFI_INSTRUCTION 0
+    %w0 = LDRHHui killed %x0, 1 :: (load 2 from %ir.dst1)
+    RET %lr, implicit %w0
+
+...
+# CHECK-LABEL: name:            promote-load-from-store
+# CHECK: STRWui killed %w1
+# CHECK: UBFMWri %w1
+---
+name:            store-pair
+alignment:       2
+exposesReturnsTwice: false
+hasInlineAsm:    false
+allVRegsAllocated: true
+isSSA:           false
+tracksRegLiveness: false
+tracksSubRegLiveness: false
+liveins:         
+  - { reg: '%x0' }
+  - { reg: '%w1' }
+frameInfo:       
+  isFrameAddressTaken: false
+  isReturnAddressTaken: false
+  hasStackMap:     false
+  hasPatchPoint:   false
+  stackSize:       0
+  offsetAdjustment: 0
+  maxAlignment:    0
+  adjustsStack:    false
+  hasCalls:        false
+  maxCallFrameSize: 0
+  hasOpaqueSPAdjustment: false
+  hasVAStart:      false
+  hasMustTailInVarArgFunc: false
+body:             |
+  bb.0 (%ir-block.0):
+    liveins: %w1, %x0
+  
+    STRWui %w1, %x0, 0 :: (store 4 into %ir.dst01)
+    CFI_INSTRUCTION 0
+    CFI_INSTRUCTION 0
+    CFI_INSTRUCTION 0
+    CFI_INSTRUCTION 0
+    CFI_INSTRUCTION 0
+    CFI_INSTRUCTION 0
+    CFI_INSTRUCTION 0
+    CFI_INSTRUCTION 0
+    CFI_INSTRUCTION 0
+    CFI_INSTRUCTION 0
+    CFI_INSTRUCTION 0
+    CFI_INSTRUCTION 0
+    CFI_INSTRUCTION 0
+    CFI_INSTRUCTION 0
+    CFI_INSTRUCTION 0
+    CFI_INSTRUCTION 0
+    CFI_INSTRUCTION 0
+    CFI_INSTRUCTION 0
+    CFI_INSTRUCTION 0
+    CFI_INSTRUCTION 0
+    STRWui killed %w1, killed %x0, 1 :: (store 4 into %ir.dst1)
+    RET %lr
+
+...
+# CHECK-LABEL: name:            store-pair
+# CHECK: STPWi