[AArch64] Avoid pairing loads with same result reg

[congzhe]

Summary

When pairing ldr instructions to an ldp instruction, we cannot pair two ldr
destination registers where one is a sub or super register of the other.

[fhahn]

llvm/test/CodeGen/AArch64/aarch64-ldst-subsuperReg-no-ldp.mir

31

Is it possible to strip the IR references from the MIR test? Ideally most/all of the IR function here can be dropped.

188

Are all the loads/stores and the different BBs needed for the test? Also, are the different functions needed or do they test different scenarios?

[congzhe]

llvm/test/CodeGen/AArch64/aarch64-ldst-subsuperReg-no-ldp.mir

188

Thanks for the comment!
The different functions do not test different scenarios. This is one single program (one single test) that exposes the bug, so probably all functions/BBs are still needed but I can try to reduce the program. I'm not very hopeful though because this test file is already a very much reduced version compared to its original version.

[congzhe]

Revision:

• Fixed code style
• Reduce the length of the test file

[congzhe]

llvm/test/CodeGen/AArch64/aarch64-ldst-subsuperReg-no-ldp.mir

31

I spent effort trying to drop out the IR, but since some global variables such as @_MergedGlobals or @i are used in the mir, if I were to drop the IR llc would report undefined global variables error. Is there a way that I can drop the IR while still being able to run the mir? @fhahn

188

Now reduced the size of the test file, resulting in only one function in the mir which exposes the LDR-to-LDP-conversion bug.

[fhahn]

llvm/test/CodeGen/AArch64/aarch64-ldst-subsuperReg-no-ldp.mir

31

I think it should be possible to get rid of the IR references by re-writing the address computations to use the same base register but with different offsets to ensure it is clear there is no aliasing going on. Reduced test case:

---
name:            test
tracksRegLiveness: true
body:             |
  bb.0:
    liveins: $x9, $x11

    renamable $w10 = LDRWui renamable $x9, 2 :: (load 4)
    STRWui killed renamable $w10, renamable $x9, 2 :: (store 4)
    renamable $x10 = LDRSWui renamable $x9, 3 :: (load 4)
    STRWui renamable $w11, renamable $x9, 8 :: (store 4)
    renamable $w10 = LDRWui renamable $x9, 2 :: (load 4)
    RET undef $lr, implicit undef $w0

...

[congzhe]

llvm/test/CodeGen/AArch64/aarch64-ldst-subsuperReg-no-ldp.mir

31

@fhahn Thank you very much for this comment, now further reduced this mir test case.

Also the mir test case in https://reviews.llvm.org/D86956 has been further reduced.

[fhahn]

LGTM, thanks!

llvm/test/CodeGen/AArch64/aarch64-ldst-subsuperReg-no-ldp.mir

13

Given that the test is very short now, could you check for the full instruction sequence (LDR, LDR, RET)? Just to make sure the loads are not dropped completely (as $w10 is not really used any more)/.

Also, it might eb good to add another test with the order of LDRSW/LDRW flipped.

[congzhe]

llvm/test/CodeGen/AArch64/aarch64-ldst-subsuperReg-no-ldp.mir

13

@fhahn Thanks, now checking the full instruction sequence;

Also added another test with the order of LDRSW/LDRW flipped.

[congzhe]

llvm/test/CodeGen/AArch64/aarch64-ldst-subsuperReg-no-ldp.mir

13

Also made similar updates in the other patch
https://reviews.llvm.org/D86956

[fhahn]

llvm/test/CodeGen/AArch64/aarch64-ldst-subsuperReg-no-ldp.mir

13

Thanks, that looks good!

[fhahn]

oh, it might be good to slightly adjust the commit message to be a bit more compact and also mention what actual bug is fixed, e.g. something like [AArch64] Avoid paring loads with same result reg.

[congzhe]

In D86906#2279815, @fhahn wrote:

oh, it might be good to slightly adjust the commit message to be a bit more compact and also mention what actual bug is fixed, e.g. something like [AArch64] Avoid paring loads with same result reg.

Thanks, updated the commit message accordingly.

[fhahn]

Great. Please let us know if you need someone to commit the change on your behalf.

llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp

if (areCandidatesToMergeOrPair(FirstMI, MI, Flags, TII) &&
           // looking.
           if (IsUnscaled && (alignTo(MinOffset, OffsetStride) != MinOffset)) {
             LiveRegUnits::accumulateUsedDefed(MI, ModifiedRegUnits,
                                               UsedRegUnits, TRI);
             MemInsns.push_back(&MI);
             continue;
           }
         }
-        // If the destination register of the loads is the same register, bail
-        // and keep looking. A load-pair instruction with both destination
-        // registers the same is UNPREDICTABLE and will result in an exception.
-        if (MayLoad && Reg == getLdStRegOp(MI).getReg()) {
+        // If the destination register of one load is the same register or a
+        // sub/super register of the other load, bail and keep looking. A
+        // load-pair instruction with both destination registers the same is
+        // UNPREDICTABLE and will result in an exception.
+        if (MayLoad &&
+            TRI->isSuperOrSubRegisterEq(Reg, getLdStRegOp(MI).getReg())) {
           LiveRegUnits::accumulateUsedDefed(MI, ModifiedRegUnits, UsedRegUnits,
                                             TRI);
           MemInsns.push_back(&MI);
           continue;
         }
 
         // If the Rt of the second instruction was not modified or used between
         // the two instructions and none of the instructions between the second

llvm/test/CodeGen/AArch64/aarch64-ldst-subsuperReg-no-ldp.mir

+# RUN: llc -mtriple=aarch64-linux-gnu -verify-machineinstrs -run-pass=aarch64-ldst-opt %s -o - | FileCheck %s
+#
+# The test below tests that when the AArch64 Load Store Optimization pass tries to
+# convert load instructions into a ldp instruction, and when the destination
+# registers are sub/super register of each other, then the convertion should not occur.
+#
+# For example, for the following pattern:
+#     ldr x10 [x9]
+#     ldr w10 [x9, 8],
+# We cannot convert it to an ldp instruction.
+#
+# CHECK-NOT: LDP
+# CHECK: $x10 = LDRSWui $x9, 0
+# CHECK: $w10 = LDRWui $x9, 1
+# CHECK: RET
+---
+name:            test1
+tracksRegLiveness: true
+body:             |
+  bb.0:
+    liveins: $x9
+    $x10 = LDRSWui $x9, 0 :: (load 4)
+    $w10 = LDRWui $x9, 1 :: (load 4)
+    RET undef $lr, implicit undef $w0
+...
+# CHECK-NOT: LDP
+# CHECK: $w10 = LDRWui $x9, 0
+# CHECK: $x10 = LDRSWui $x9, 1
+# CHECK: RET
+---
+name:            test2
+tracksRegLiveness: true
+body:             |
+  bb.0:
+    liveins: $x9
+    $w10 = LDRWui $x9, 0 :: (load 4)
+    $x10 = LDRSWui $x9, 1 :: (load 4)
+    RET undef $lr, implicit undef $w0
+...