[AArch64] Avoid pairing loads when the base reg is modified

[congzhe]

Summary

When pairing loads, we should check if in between the two loads the
base register has been modified. If that is the case then avoid pairing
them because the second load actually loads from a different address.

[fhahn]

Is it possible to add a MIR test for the issue? Also, it looks like the formatting is off a bit. Could you run clang-format-diff on the change?

[congzhe]

In D86956#2249916, @fhahn wrote:

Is it possible to add a MIR test for the issue? Also, it looks like the formatting is off a bit. Could you run clang-format-diff on the change?

Thanks! Regarding the MIR test: similar to the test file in the other patch https://reviews.llvm.org/D86906, the MIR test file for this patch is very lengthy (over 1000 lines). I'm wondering if it looks fine if I use that test file, or I could spend time trying to prune it, although I'm not optimistic how much it can be reduced.

[congzhe]

Revision:

• Fixed code style
• Provided an mir test

[congzhe]

In D86956#2249916, @fhahn wrote:

Is it possible to add a MIR test for the issue? Also, it looks like the formatting is off a bit. Could you run clang-format-diff on the change?

Thanks for the comment, now fixed the code style and provided a test case which has already been reduced using creduce and llvm-reduce.

[congzhe]

Further reduced the mir test case.

[fhahn]

llvm/test/CodeGen/AArch64/aarch64-ldst-modified-baseReg.mir

17 ↗

(On Diff #292460)

It might be good to have test cases with stores & non-load instructions modifying the base?

[congzhe]

llvm/test/CodeGen/AArch64/aarch64-ldst-modified-baseReg.mir

17 ↗

(On Diff #292460)

Thanks for this comment, now added test cases where the base register or base address is modified with non-load instructions.
However, the bug in AArch64 Load Store Optimization pass only exists when the base register is updated with load instructions in between two loads for pairing, i.e.,

ldr x9 [x10]
ldr x10 [x8]
ldr x10 [x10, 8].

For other instructions that modify the base register or base address with non-load instructions, the existing pass works correctly. The case that the base register is modified with non-load instructions is handled in lines 1626-1627 of the original AArch64 Load Store Optimization pass:

if (!ModifiedRegUnits.available(BaseReg))
  return E;

Only when the pattern shown above occurs, this pass fails to handle it because it would hit a continue in the for-loop and would not reach lines 1626-1627.

Still, I added these test cases where the base register or base address is modified with non-load instructions, but please note that these test cases already work correctly without the patch since the original AArch64 Load Store Optimization pass handles them well. I did not find similar tests in other test files that's why I added them, but maybe not adding them also makes sense. I'll appreciate it if you could let me know your thoughts.

[congzhe]

@fhahn pinging reviewers :)
Comments are very much appreciated.

[fhahn]

llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp

1572

then *we* cannot do the optimization?

1578

can we hoist the check outside of the containing if, i.e. to around line 1562? I think we should be able to bail out once the base reg is modified, because it won't get 'un-modified' so that should not rule out any valid pairs.

Also, it is safer to do it earlier, otherwise we would need the check for each code path that returns a valid found pair (for example, we would probably also need it around line 1611)

llvm/test/CodeGen/AArch64/aarch64-ldst-modified-baseReg.mir

3 ↗

(On Diff #293056)

nit: spurious to, should that be tries to convert load instructions?

4 ↗

(On Diff #293056)

a ldp instruction?

5 ↗

(On Diff #293056)

convertion -> conversion?

17 ↗

(On Diff #292460)

Thanks for adding those tests! Even though it is already handled correctly, adding a few additional tests here for completeness makes sense to me.

[fhahn]

LGTM, thanks!

llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp

1564

nit: capitalize For.

[congzhe]

Thanks for all the comments, now addressed all of them @fhahn

llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp

1572

Thanks, updated accordingly.

1578

Now hoisted the check outside of the containing if.

llvm/test/CodeGen/AArch64/aarch64-ldst-modified-baseReg.mir

5 ↗

(On Diff #293056)

Thanks for the comments, revised accordingly.

llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp

if (areCandidatesToMergeOrPair(FirstMI, MI, Flags, TII) &&
         if (MayLoad && 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
         // and first alias with the second, we can combine the second into the
         // first.
         if (ModifiedRegUnits.available(getLdStRegOp(MI).getReg()) &&
             !(MI.mayLoad() &&
               !UsedRegUnits.available(getLdStRegOp(MI).getReg())) &&
             !mayAlias(MI, MemInsns, AA)) {
 
+          // If the BaseReg has been modified, then cannot do the optimization.
+          // for example, in the following pattern
+          //   ldr x1 [x2]
+          //   ldr x2 [x3]
+          //   ldr x4 [x2, #8],
+          // the first and third ldr cannot be converted to ldp x1, x4, [x2]
+    if (!ModifiedRegUnits.available(BaseReg))
+            return E;
           Flags.setMergeForward(false);
           Flags.clearRenameReg();
           return MBBI;
         }
 
         // Likewise, if the Rt of the first instruction is not modified or used
         // between the two instructions and none of the instructions between the
         // first and the second alias with the first, we can combine the first
         // into the second.
         if (!(MayLoad &&
               !UsedRegUnits.available(getLdStRegOp(FirstMI).getReg())) &&
             !mayAlias(FirstMI, MemInsns, AA)) {
 
           if (ModifiedRegUnits.available(getLdStRegOp(FirstMI).getReg())) {
+      if (!ModifiedRegUnits.available(BaseReg))
+              return E;
             Flags.setMergeForward(true);
             Flags.clearRenameReg();
             return MBBI;
           }
 
           if (DebugCounter::shouldExecute(RegRenamingCounter)) {
             if (!MaybeCanRename)
               MaybeCanRename = {canRenameUpToDef(FirstMI, UsedInBetween,