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 ↗

(On Diff #84896)

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 ↗

(On Diff #84896)

Don't we need to set kill on it ?

699–700 ↗

(On Diff #84896)

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

test/CodeGen/AArch64/ldst-opt.mir

132 ↗

(On Diff #84896)

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 ↗

(On Diff #84896)

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 ↗

(On Diff #84896)

Kills are optional (see above)

699–700 ↗

(On Diff #84896)

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

test/CodeGen/AArch64/ldst-opt.mir

132 ↗

(On Diff #84896)

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 ↗

(On Diff #84896)

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 ↗

(On Diff #84896)

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.

llvm/trunk/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()));

llvm/trunk/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

llvm/trunk/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