AArch64LoadStoreOptimizer: Update kill flags when merging stores

[MatzeB]

Summary

Kill flags need to be updated correctly when moving stores up/down to
form store pair instructions.
Those invalid flags have been ignored before but as of r290014 they are
recognized when using -mllvm -verify-machineinstrs.

Also simplifies test/CodeGen/AArch64/ldst-opt-dbg-limit.mir, renames it
to ldst-opt.mir test and adds a new tests for this change.

[junbuml]

lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp

695

As far as I see, only store pair seem to be the issue, but just want to confirm if there is any case for load pair ?

697

Don't we need to set kill on it ?

699–700

If there is no instruction using RegOp0 and RegOp1 between stores, no need to clear kill.

test/CodeGen/AArch64/ldst-opt.mir

133

Don't we need to set kill on w1 here ?

[MatzeB]

Thanks for the review.

The thing with Kill flags is that we want to get rid of them long term. Today they are optional: The last use of a value may or may not have a kill flag set, but if there is a kill flag, then the value must be killed at that point.
This patch fixes situations in which invalid kill flags are produced, it can remove more kill flags than necessary or not adding possible new ones; But that is fine and keeps the code simpler.

lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp

695

Paired loads are fine, the loaded values are defs and the base register is taken from the place where the final paired instruction will end up (const MachineOperand &BaseRegOp = MergeForward ? getLdStBaseOp(*Paired) : getLdStBaseOp(*I);) so the kill flags are correct for that.

697

Kills are optional (see above)

699–700

Yes there is no need, but it doesn't hurt either and keeps the code slightly simpler.

test/CodeGen/AArch64/ldst-opt.mir

133

Kills are optional (see above).

[mcrosier]

Given we want to deprecate the use of kill flags and AFAIK there are no other passes that use these flags after the load/store optimizer, I'm okay with the lossiness of this solution.

lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp

707

If the two instructions being paired happen to be adjacent to one another, NextI will point to the instruction after 'Paired'. Is this going to behave as expected with make_range()?

[MatzeB]

Thanks for the review, address comment.

[MatzeB]

lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp

707

Good point, for some reason that case never happened if MergeForward was true, but what I really should be used here is std::next(I).

[mcrosier]

LGTM.

lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp

assert(!(OffsetImm % getMemScale(*RtMI)) &&
            "Unscaled offset cannot be scaled.");
     OffsetImm /= getMemScale(*RtMI);
   }
 
   // Construct the new instruction.
   MachineInstrBuilder MIB;
   DebugLoc DL = I->getDebugLoc();
   MachineBasicBlock *MBB = I->getParent();
+  MachineOperand RegOp0 = getLdStRegOp(*RtMI);
+  MachineOperand RegOp1 = getLdStRegOp(*Rt2MI);
+  // Kill flags may become invalid when moving stores for pairing.
+  if (RegOp0.isUse()) {
+    if (!MergeForward) {
+      // Clear kill flags on store if moving upwards. Example:
+      //   STRWui %w0, ...
+      //   USE %w1
+      //   STRWui kill %w1  ; need to clear kill flag when moving STRWui upwards
+      RegOp0.setIsKill(false);
+      RegOp1.setIsKill(false);
+    } else {
+      // Clear kill flags of the first stores register. Example:
+      //   STRWui %w1, ...
+      //   USE kill %w1   ; need to clear kill flag when moving STRWui downwards
+      //   STRW %w0
+      unsigned Reg = getLdStRegOp(*I).getReg();
+      for (MachineInstr &MI : make_range(std::next(I), Paired))
+        MI.clearRegisterKills(Reg, TRI);
+    }
+  }
   MIB = BuildMI(*MBB, InsertionPoint, DL, TII->get(getMatchingPairOpcode(Opc)))
-            .add(getLdStRegOp(*RtMI))
-            .add(getLdStRegOp(*Rt2MI))
+            .add(RegOp0)
+            .add(RegOp1)
             .add(BaseRegOp)
             .addImm(OffsetImm)
             .setMemRefs(I->mergeMemRefsWith(*Paired));
 
   (void)MIB;
 
   DEBUG(dbgs() << "Creating pair load/store. Replacing instructions:\n    ");
   DEBUG(I->print(dbgs()));

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
-tracksRegLiveness: true
-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, %lr
-  
-    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 %w1
-# CHECK: UBFMWri %w1
----
-name:            store-pair
-alignment:       2
-exposesReturnsTwice: false
-tracksRegLiveness: true
-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, %lr
-  
-    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

test/CodeGen/AArch64/ldst-opt.mir

+# RUN: llc -mtriple=aarch64--linux-gnu -run-pass=aarch64-ldst-opt %s -verify-machineinstrs -o - 2>&1 | FileCheck %s
+--- |
+  define void @promote-load-from-store() { ret void }
+  define void @store-pair() { ret void }
+  define void @store-pair-clearkill0() { ret void }
+  define void @store-pair-clearkill1() { ret void }
+...
+---
+name: promote-load-from-store
+tracksRegLiveness: true
+body: |
+  bb.0:
+    liveins: %w1, %x0, %lr
+
+    STRWui killed %w1, %x0, 0 :: (store 4)
+    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)
+    RET %lr, implicit %w0
+
+...
+# Don't count transient instructions towards search limits.
+# CHECK-LABEL: name: promote-load-from-store
+# CHECK: STRWui %w1
+# CHECK: UBFMWri %w1
+---
+name: store-pair
+tracksRegLiveness: true
+body: |
+  bb.0:
+    liveins: %w1, %x0, %lr
+
+    STRWui %w1, %x0, 0 :: (store 4)
+    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)
+    RET %lr
+
+...
+# CHECK-LABEL: name: store-pair
+# CHECK: STPWi
+---
+name: store-pair-clearkill0
+tracksRegLiveness: true
+body: |
+  bb.0:
+    liveins: %w1, %x0, %lr
+
+    STRWui %w1, %x0, 0 :: (store 4)
+    %w2 = COPY %w1
+    %x3 = COPY %x0
+    STRWui killed %w1, killed %x0, 1 :: (store 4)
+    RET %lr
+...
+# When merging a lower store with an upper one, we must clear kill flags on
+# the lower store.
+# CHECK-LABEL: store-pair-clearkill0
+# CHECK-NOT: STPWi %w1, killed %w1, %x0, 0 :: (store 4)
+# CHECK: STPWi %w1, %w1, %x0, 0 :: (store 4)
+# CHECK: %w2 = COPY %w1
+# CHECK: RET %lr
+---
+name: store-pair-clearkill1
+tracksRegLiveness: true
+body: |
+  bb.0:
+    liveins: %x0, %lr
+
+    %w1 = MOVi32imm 13
+    %w2 = MOVi32imm 7
+    STRWui %w1, %x0, 1 :: (store 4)
+    %w2 = COPY killed %w1
+    STRWui killed %w2, %x0, 0 :: (store 4)
+
+    %w1 = MOVi32imm 42
+    %w2 = MOVi32imm 7
+    STRWui %w1, %x0, 0 :: (store 4)
+    %w2 = COPY killed %w1
+    STRWui killed %w2, killed %x0, 1 :: (store 4)
+
+    RET %lr
+...
+# When merging an upper store with a lower one, kill flags along the way need
+# to be removed; In this case the kill flag on %w1.
+# CHECK-LABEL: store-pair-clearkill1
+# CHECK: %w1 = MOVi32imm
+# CHECK: %w2 = MOVi32imm
+# CHECK-NOT: %w2 = COPY killed %w1
+# CHECK: %w2 = COPY %w1
+# CHECK: STPWi killed %w2, %w1, %x0, 0
+
+# CHECK: %w1 = MOVi32imm
+# CHECK: %w2 = MOVi32imm
+# CHECK-NOT: %w2 = COPY killed %w1
+# CHECK: %w2 = COPY %w1
+# CHECK: STPWi %w1, killed %w2, killed %x0, 0