[AArch64] Merge LDRSWpre-LD[U]RSW pair into LDPSWpre.

[chaosdefinition]

Summary

This patch optimizes a pair of LDRSWpre and LDRSWui (or LDURSWi) instructions into a single LDPSWpre instruction. This is a missing case in D99272.

MIR test cases in D152564 are updated to verify the optimization.

[fhahn]

llvm/test/CodeGen/AArch64/ldrpre-ldr-merge.mir

608

I think we should also have a test case with LDURSWi and variants where LDRSWpre is the second merge candidate. Preferably the tests will be submitted separately first.

[chaosdefinition]

llvm/test/CodeGen/AArch64/ldrpre-ldr-merge.mir

608

Thanks for reviewing! By submitting the tests first, I suppose merge cases should be no merge to avoid test failures and then updated to merge here, correct?

[chaosdefinition]

Addressing comments by @fhahn:

• Add a test case of merging LDRSWpre and LDURSWi
• Add test cases with LDRSWpre as the second merge candidate
• Split test cases into a separate patch (see D152564)

[dmgreen]

Can you add a test where getMatchingNonSExtOpcode would be true, and we could turn into a ldp+sext? I'm not sure if it will actually happen at the moment with pre nodes. LDRSWpre + LDRWui and maybe LDRWpre + LDRSWui

[chaosdefinition]

Update the patch to be based on the updated D152564, no functionality change.

[chaosdefinition]

In D152407#4423993, @dmgreen wrote:

Can you add a test where getMatchingNonSExtOpcode would be true, and we could turn into a ldp+sext? I'm not sure if it will actually happen at the moment with pre nodes. LDRSWpre + LDRWui and maybe LDRWpre + LDRSWui

Addressed in D152564. Indeed these are currently not merged; perhaps fix them in the future?

[chaosdefinition]

Ping...

[dmgreen]

Sorry for the delay. This fell off my radar. The changes LGTM, as far as I can see.

[eaeltsin]

Heads-up - this caused multiple segmentation faults in our code base. We are working on a reproducer.

[scw]

We see a miscompilation after this patch in protobuf under MSan. This is the line where the miscompilation happens, and I extracted it to this godbolt with compilation option -fsanitize=memory -O.

Before this patch, the AArch64 assembly is (registers are renamed a bit, because this is disassembled from a binary I built internally)

1bfbbc64: b8808d49      ldrsw   x9, [x10, #8]!
1bfbbc68: ca0d014e      eor     x14, x10, x13
1bfbbc6c: b8404d4c      ldr     w12, [x10, #4]!
1bfbbc70: ca0d014d      eor     x13, x10, x13

After, it became

1bfbb51c: 29c13149      ldp     w9, w12, [x10, #8]!
1bfbb520: 93407d4a      sxtw    x10, w10
1bfbb524: ca0d014e      eor     x14, x10, x13
1bfbb528: ca0d014d      eor     x13, x10, x13

There are two problems:

1. The after sequence touches x10, before calculating x13 and x14, which are supposed to be the addresses of the shadow memory. This caused the crash because our x10 was pointing to stack where the 32-th bit is 1. I believe that line should have been sxtw x9, w9 instead, to perform the extension done in ldrsw in the before sequence.
2. This also miscalculates x13, where in the before sequence, after execution, it holds old(x13) xor (x10 + 12), yet after the "after" sequence, it holds old(x13) xor (x10 + 8), the same as x14.

[chaosdefinition]

In D152407#4531841, @eaeltsin wrote:

Heads-up - this caused multiple segmentation faults in our code base. We are working on a reproducer.

In D152407#4533478, @scw wrote:

We see a miscompilation after this patch in protobuf under MSan. This is the line where the miscompilation happens, and I extracted it to this godbolt with compilation option -fsanitize=memory -O.

Before this patch, the AArch64 assembly is (registers are renamed a bit, because this is disassembled from a binary I built internally)

1bfbbc64: b8808d49      ldrsw   x9, [x10, #8]!
1bfbbc68: ca0d014e      eor     x14, x10, x13
1bfbbc6c: b8404d4c      ldr     w12, [x10, #4]!
1bfbbc70: ca0d014d      eor     x13, x10, x13

After, it became

1bfbb51c: 29c13149      ldp     w9, w12, [x10, #8]!
1bfbb520: 93407d4a      sxtw    x10, w10
1bfbb524: ca0d014e      eor     x14, x10, x13
1bfbb528: ca0d014d      eor     x13, x10, x13

There are two problems:

1. The after sequence touches x10, before calculating x13 and x14, which are supposed to be the addresses of the shadow memory. This caused the crash because our x10 was pointing to stack where the 32-th bit is 1. I believe that line should have been sxtw x9, w9 instead, to perform the extension done in ldrsw in the before sequence.
2. This also miscalculates x13, where in the before sequence, after execution, it holds old(x13) xor (x10 + 12), yet after the "after" sequence, it holds old(x13) xor (x10 + 8), the same as x14.

Thanks for reporting and reproducing the miscompilation! It looks like two pre-indexed loads LDRSWpre and LDRWpre got merged into a LDPWpre and SBFMXri, which is an unintended result by this patch. I think we can either rollback this patch or apply a quick fix that disallows merging two pre-indexed loads like the following. Any thoughts?

diff --git a/llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp b/llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp
index 419b471db3a3..e64b1d0658f0 100644
--- a/llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp
+++ b/llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp
@@ -1317,20 +1317,24 @@ static bool areCandidatesToMergeOrPair(MachineInstr &FirstMI, MachineInstr &MI,
                                   MI.getFlag(MachineInstr::FrameDestroy)))
     return false;

   unsigned OpcA = FirstMI.getOpcode();
   unsigned OpcB = MI.getOpcode();

   // Opcodes match: If the opcodes are pre ld/st there is nothing more to check.
   if (OpcA == OpcB)
     return !AArch64InstrInfo::isPreLdSt(FirstMI);

+  // Two pre ld/st of different opcodes cannot be merged either
+  if (AArch64InstrInfo::isPreLdSt(FirstMI) && AArch64InstrInfo::isPreLdSt(MI))
+    return false;
+
   // Try to match a sign-extended load/store with a zero-extended load/store.
   bool IsValidLdStrOpc, PairIsValidLdStrOpc;
   unsigned NonSExtOpc = getMatchingNonSExtOpcode(OpcA, &IsValidLdStrOpc);
   assert(IsValidLdStrOpc &&
          "Given Opc should be a Load or Store with an immediate");
   // OpcA will be the first instruction in the pair.
   if (NonSExtOpc == getMatchingNonSExtOpcode(OpcB, &PairIsValidLdStrOpc)) {
     Flags.setSExtIdx(NonSExtOpc == (unsigned)OpcA ? 1 : 0);
     return true;
   }

[asmok-g]

Thanks for reporting and reproducing the miscompilation! It looks like two pre-indexed loads LDRSWpre and LDRWpre got merged into a LDPWpre and SBFMXri, which is an unintended result by this patch. I think we can either rollback this patch or apply a quick fix that disallows merging two pre-indexed loads like the following. Any thoughts?

Hi there! I'm not an expert here, but speaking with @scw and he thinks the fix doesn't address the "sxtw on wrong register bug". He'll probably be able to comment with details later but his vote is to rollback for now. Can you please do ?

Thanks

[alexfh]

In D152407#4534575, @asmok-g wrote:

Thanks for reporting and reproducing the miscompilation! It looks like two pre-indexed loads LDRSWpre and LDRWpre got merged into a LDPWpre and SBFMXri, which is an unintended result by this patch. I think we can either rollback this patch or apply a quick fix that disallows merging two pre-indexed loads like the following. Any thoughts?

Hi there! I'm not an expert here, but speaking with @scw and he thinks the fix doesn't address the "sxtw on wrong register bug". He'll probably be able to comment with details later but his vote is to rollback for now. Can you please do ?

Thanks

Sent a revert: https://reviews.llvm.org/D156328 (will commit after the precommit checks finish).

[chaosdefinition]

llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp

1334

In D152407#4534575, @asmok-g wrote:

Hi there! I'm not an expert here, but speaking with @scw and he thinks the fix doesn't address the "sxtw on wrong register bug". He'll probably be able to comment with details later but his vote is to rollback for now. Can you please do ?

Thanks

The wrong register in SBFMXri was resulted from SExtIdx being set to 0 here. However, to reach here:

• OpcA and OpcB must be different;
• OpcA and OpcB must have the same NonSExtOpcode.

So this code path works only for pairs of LDRSWui-LDRWui and LDURSWi-LDURWi before.

Now with this patch, the only added case to reach here is LDRSWpre-LDRWpre pairs, as was caught by the miscompilation. So my quick fix should prevent such pairs from generating SBFMXri, by bailing out early on two pre-indexed loads.

I guess there might be a better way to fix this. Any thoughts while I work on an updated patch?

Thanks!

[dmgreen]

Reverting in the sort term does sounds like a good idea to make sure LLVM 17 is in a good shape. We can then work on getting this recommitted with the fix.

[chaosdefinition]

Hi, I'm reopening this with the following updates:

• Apply the quick fix described in D152407#4534022 that disallows merging two pre-indexed loads, assuming there's no better approach.
• Add MIR regression tests modified from @scw's reproduction .

Thanks!

[scw]

In D152407#4619082, @chaosdefinition wrote:

Hi, I'm reopening this with the following updates:

• Apply the quick fix described in D152407#4534022 that disallows merging two pre-indexed loads, assuming there's no better approach.
• Add MIR regression tests modified from @scw's reproduction .

Thanks!

Test case LGTM, I don't have enough knowledge for the code to know if the proposed fix is the best way though.

Also, should this change reuse the same D number or should we create a new one?

[chaosdefinition]

In D152407#4625772, @scw wrote:

Test case LGTM, I don't have enough knowledge for the code to know if the proposed fix is the best way though.

Also, should this change reuse the same D number or should we create a new one?

I don't know what the standard way for this is. I'm inclined to reuse the D number so to keep the discussion in this thread, but opening up a new thread is fine as well.

[dmgreen]

The same revision should be OK, if the change is just a fix to the existing patch.

I had a look through the LoadStoreOptimizer. If you don't think there are any other cases this can go wrong them it LGTM.

[chaosdefinition]

In D152407#4642721, @dmgreen wrote:

The same revision should be OK, if the change is just a fix to the existing patch.

I had a look through the LoadStoreOptimizer. If you don't think there are any other cases this can go wrong them it LGTM.

Thanks. I don't see any other cases that can go wrong with this. Will commit shortly.

llvm/lib/Target/AArch64/AArch64InstrInfo.cpp

bool AArch64InstrInfo::hasUnscaledLdStOffset(unsigned Opc) {
   case AArch64::LDURDi:
   case AArch64::LDRDpre:
   case AArch64::LDURQi:
   case AArch64::LDRQpre:
   case AArch64::LDURWi:
   case AArch64::LDRWpre:
   case AArch64::LDURXi:
   case AArch64::LDRXpre:
+  case AArch64::LDRSWpre:
   case AArch64::LDURSWi:
   case AArch64::LDURHHi:
   case AArch64::LDURBBi:
   case AArch64::LDURSBWi:
   case AArch64::LDURSHWi:
     return true;
   }
 }
bool AArch64InstrInfo::isPairableLdStInst(const MachineInstr &MI) {
   case AArch64::LDRDpre:
   case AArch64::LDURQi:
   case AArch64::LDRQpre:
   case AArch64::LDURWi:
   case AArch64::LDRWpre:
   case AArch64::LDURXi:
   case AArch64::LDRXpre:
   case AArch64::LDURSWi:
+  case AArch64::LDRSWpre:
     return true;
   }
 }
 
 unsigned AArch64InstrInfo::convertToFlagSettingOpc(unsigned Opc) {
   switch (Opc) {
   default:
     llvm_unreachable("Opcode has no flag setting equivalent!");
bool AArch64InstrInfo::isCandidateToMergeOrPair(const MachineInstr &MI) const {
   bool IsImmPreLdSt = IsPreLdSt && MI.getOperand(3).isImm();
 
   if (!MI.getOperand(2).isImm() && !IsImmPreLdSt)
     return false;
 
   // Can't merge/pair if the instruction modifies the base register.
   // e.g., ldr x0, [x0]
   // This case will never occur with an FI base.
-  // However, if the instruction is an LDR/STR<S,D,Q,W,X>pre, it can be merged.
+  // However, if the instruction is an LDR<S,D,Q,W,X,SW>pre or
+  // STR<S,D,Q,W,X>pre, it can be merged.
   // For example:
   //   ldr q0, [x11, #32]!
   //   ldr q1, [x11, #16]
   //   to
   //   ldp q0, q1, [x11, #32]!
   if (MI.getOperand(1).isReg() && !IsPreLdSt) {
     Register BaseReg = MI.getOperand(1).getReg();
     const TargetRegisterInfo *TRI = &getRegisterInfo();
int AArch64InstrInfo::getMemScale(unsigned Opc) {
   case AArch64::STRHHui:
   case AArch64::STURHHi:
     return 2;
   case AArch64::LDRSui:
   case AArch64::LDURSi:
   case AArch64::LDRSpre:
   case AArch64::LDRSWui:
   case AArch64::LDURSWi:
+  case AArch64::LDRSWpre:
   case AArch64::LDRWpre:
   case AArch64::LDRWui:
   case AArch64::LDURWi:
   case AArch64::STRSui:
   case AArch64::STURSi:
   case AArch64::STRSpre:
   case AArch64::STRWui:
   case AArch64::STURWi:

 }
 
 bool AArch64InstrInfo::isPreLd(const MachineInstr &MI) {
   switch (MI.getOpcode()) {
   default:
     return false;
   case AArch64::LDRWpre:
   case AArch64::LDRXpre:
+  case AArch64::LDRSWpre:
   case AArch64::LDRSpre:
   case AArch64::LDRDpre:
   case AArch64::LDRQpre:
     return true;
   }
 }
 
 bool AArch64InstrInfo::isPreSt(const MachineInstr &MI) {

llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp

static unsigned getMatchingNonSExtOpcode(unsigned Opc,
   case AArch64::LDRSui:
   case AArch64::LDURSi:
   case AArch64::LDRSpre:
     return Opc;
   case AArch64::LDRSWui:
     return AArch64::LDRWui;
   case AArch64::LDURSWi:
     return AArch64::LDURWi;
+  case AArch64::LDRSWpre:
+    return AArch64::LDRWpre;
   }
 }
 
 static unsigned getMatchingWideOpcode(unsigned Opc) {
   switch (Opc) {
   default:
     llvm_unreachable("Opcode has no wide equivalent!");
   case AArch64::STRBBui:
static unsigned getMatchingPairOpcode(unsigned Opc) {
   case AArch64::LDRXui:
   case AArch64::LDURXi:
     return AArch64::LDPXi;
   case AArch64::LDRXpre:
     return AArch64::LDPXpre;
   case AArch64::LDRSWui:
   case AArch64::LDURSWi:
     return AArch64::LDPSWi;
+  case AArch64::LDRSWpre:
+    return AArch64::LDPSWpre;
   }
 }
 
 static unsigned isMatchingStore(MachineInstr &LoadInst,
                                 MachineInstr &StoreInst) {
   unsigned LdOpc = LoadInst.getOpcode();
   unsigned StOpc = StoreInst.getOpcode();
   switch (LdOpc) {
static bool isPreLdStPairCandidate(MachineInstr &FirstMI, MachineInstr &MI) {
   case AArch64::LDRDpre:
     return (OpcB == AArch64::LDRDui) || (OpcB == AArch64::LDURDi);
   case AArch64::LDRQpre:
     return (OpcB == AArch64::LDRQui) || (OpcB == AArch64::LDURQi);
   case AArch64::LDRWpre:
     return (OpcB == AArch64::LDRWui) || (OpcB == AArch64::LDURWi);
   case AArch64::LDRXpre:
     return (OpcB == AArch64::LDRXui) || (OpcB == AArch64::LDURXi);
+  case AArch64::LDRSWpre:
+    return (OpcB == AArch64::LDRSWui) || (OpcB == AArch64::LDURSWi);
   }
 }
 
 // Returns the scale and offset range of pre/post indexed variants of MI.
 static void getPrePostIndexedMemOpInfo(const MachineInstr &MI, int &Scale,
                                        int &MinOffset, int &MaxOffset) {
   bool IsPaired = AArch64InstrInfo::isPairedLdSt(MI);
   bool IsTagStore = isTagStore(MI);
static bool areCandidatesToMergeOrPair(MachineInstr &FirstMI, MachineInstr &MI,
 
   // Try to match a sign-extended load/store with a zero-extended load/store.
   bool IsValidLdStrOpc, PairIsValidLdStrOpc;
   unsigned NonSExtOpc = getMatchingNonSExtOpcode(OpcA, &IsValidLdStrOpc);
   assert(IsValidLdStrOpc &&
          "Given Opc should be a Load or Store with an immediate");
   // OpcA will be the first instruction in the pair.
   if (NonSExtOpc == getMatchingNonSExtOpcode(OpcB, &PairIsValidLdStrOpc)) {
     Flags.setSExtIdx(NonSExtOpc == (unsigned)OpcA ? 1 : 0);
     return true;
   }
 
   // If the second instruction isn't even a mergable/pairable load/store, bail
   // out.
   if (!PairIsValidLdStrOpc)
     return false;
 
   // FIXME: We don't support merging narrow stores with mixed scaled/unscaled
   // offsets.
   if (isNarrowStore(OpcA) || isNarrowStore(OpcB))
     return false;
 
   // The STR<S,D,Q,W,X>pre - STR<S,D,Q,W,X>ui and
-  // LDR<S,D,Q,W,X>pre-LDR<S,D,Q,W,X>ui
+  // LDR<S,D,Q,W,X,SW>pre-LDR<S,D,Q,W,X,SW>ui
   // are candidate pairs that can be merged.
   if (isPreLdStPairCandidate(FirstMI, MI))
     return true;
 
   // Try to match an unscaled load/store with a scaled load/store.
   return TII->hasUnscaledLdStOffset(OpcA) != TII->hasUnscaledLdStOffset(OpcB) &&
          getMatchingPairOpcode(OpcA) == getMatchingPairOpcode(OpcB);
 

llvm/test/CodeGen/AArch64/ldrpre-ldr-merge.mir

bb.0:
     renamable $s1 = LDRSui renamable $x1, 1 :: (load (s32))
     STRSui killed renamable $s0, renamable $x1, 0 :: (store (s32))
     STRSui killed renamable $s1, renamable $x1, 1 :: (store (s32))
     RET undef $lr
 ...
 
 
 ---
-name:            21-ldrswpre-ldrswui-no-merge
+name:            21-ldrswpre-ldrswui-merge
 tracksRegLiveness: true
 liveins:
   - { reg: '$x0' }
   - { reg: '$x1' }
   - { reg: '$x2' }
 machineFunctionInfo:
   hasRedZone:      false
 body:             |
   bb.0:
     liveins: $x0, $x1, $x2
-    ; CHECK-LABEL: name: 21-ldrswpre-ldrswui-no-merge
+    ; CHECK-LABEL: name: 21-ldrswpre-ldrswui-merge
     ; CHECK: liveins: $x0, $x1, $x2
-    ; CHECK: early-clobber renamable $x1, renamable $x0 = LDRSWpre renamable $x1, 40, implicit $w1 :: (load (s32))
-    ; CHECK: renamable $x2 = LDRSWui renamable $x1, 1 :: (load (s32))
+    ; CHECK: early-clobber $x1, renamable $x0, renamable $x2 = LDPSWpre renamable $x1, 10 :: (load (s32))
     ; CHECK: STPXi renamable $x0, renamable $x2, renamable $x1, 0 :: (store (s64))
     ; CHECK: RET undef $lr
     early-clobber renamable $x1, renamable $x0 = LDRSWpre killed renamable $x1, 40 :: (load (s32))
     renamable $x2 = LDRSWui renamable $x1, 1 :: (load (s32))
     STRXui killed renamable $x0, renamable $x1, 0 :: (store (s64))
     STRXui killed renamable $x2, renamable $x1, 1 :: (store (s64))
     RET undef $lr
 ...
 
 
 ---
-name:            22-ldrswpre-ldurswi-no-merge
+name:            22-ldrswpre-ldurswi-merge
 tracksRegLiveness: true
 liveins:
   - { reg: '$x0' }
   - { reg: '$x1' }
   - { reg: '$x2' }
 machineFunctionInfo:
   hasRedZone:      false
 body:             |
   bb.0:
     liveins: $x0, $x1, $x2
-    ; CHECK-LABEL: name: 22-ldrswpre-ldurswi-no-merge
+    ; CHECK-LABEL: name: 22-ldrswpre-ldurswi-merge
     ; CHECK: liveins: $x0, $x1, $x2
-    ; CHECK: early-clobber renamable $x1, renamable $x0 = LDRSWpre renamable $x1, 40, implicit $w1 :: (load (s32))
-    ; CHECK: renamable $x2 = LDURSWi renamable $x1, 4 :: (load (s32))
+    ; CHECK: early-clobber $x1, renamable $x0, renamable $x2 = LDPSWpre renamable $x1, 10 :: (load (s32))
     ; CHECK: STPXi renamable $x0, renamable $x2, renamable $x1, 0 :: (store (s64))
     ; CHECK: RET undef $lr
     early-clobber renamable $x1, renamable $x0 = LDRSWpre killed renamable $x1, 40 :: (load (s32))
     renamable $x2 = LDURSWi renamable $x1, 4 :: (load (s32))
     STRXui killed renamable $x0, renamable $x1, 0 :: (store (s64))
     STRXui killed renamable $x2, renamable $x1, 1 :: (store (s64))
     RET undef $lr
 ...