[AArch64] Fix ldst-opt of multiple disjunct subregs.

[fhahn]

Summary

Currently aarch64-ldst-opt will incorrectly rename registers with
multiple disjunct subregisters (e.g. result of LD3). This patch updates
the canRenameUpToDef to bail out if it encounters such a register class
that contains the register to rename.

Fixes PR46105.

[efriedma]

llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp

1296

The fact that this doesn't whitelist specific opcodes makes me suspicious this is overlooking other potential issues. Some instructions with two outputs require that they aren't equal. A call instruction modifies a fixed register. Some instructions have restricted register classes.

[fhahn]

llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp

1296

I initially thought relying on renamable should be enough for the cases mentioned above. The code to pick registers should already only pick out of the most restrictive register class. It seemed to work quite well in practice so far. The problem with the disjunct sub register case is that those are represented as a single register in the result, but with as individual register when accessed by their users.

But it would probably be safer to have a whitelist. Do you know if there's a convenient way to do so for machine instructions, other than providing a really long list of opcodes?

[efriedma]

llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp

1296

Hmm, you're right, "renamable" should have the right meaning.

Maybe worth clarifying the rules on whether isRenamable() should return true for something like the AArch64 LDP, where the two results have to be placed in distinct registers. I guess you could argue renaming two defs to the same register should never be legal anyway. even if the instructions set attaches some meaning to it. But I'm not sure this pass handles it correctly, in that case. (In particular, I'm concerned what would happen if one of the results of an ldp was marked "undef".)

That said, this check still seems a little fragile: it's depending on the specific structure of the AArch64 register file. In particular, the fact that you can't overwrite a "subregister" without overwriting the whole register. Probably worth explicitly noting.

[fhahn]

llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp

1296

Maybe worth clarifying the rules on whether isRenamable() should return true for something like the AArch64 LDP, where the two results have to be placed in distinct registers. I guess you could argue renaming two defs to the same register should never be legal anyway. even if the instructions set attaches some meaning to it. But I'm not sure this pass handles it correctly, in that case. (In particular, I'm concerned what would happen if one of the results of an ldp was marked "undef".)

I think the way we choose rename registers prevents using a register that is defined by the same instruction we are renaming. I can see if I can add a test case for that.

That said, this check still seems a little fragile: it's depending on the specific structure of the AArch64 register file. In particular, the fact that you can't overwrite a "subregister" without overwriting the whole register. Probably worth explicitly noting.

I agree it is a bit fragile unfortunately, as it is easy to miss subtleties of AArch64 MIR, but I guess the only way around that would be a whitelist. Would you be happy with the current approach (with an added note as you suggested?)

[efriedma]

llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp

1296

Would you be happy with the current approach (with an added note as you suggested?)

It's probably okay if you call it out.

In the future, that might be something we should consider explicitly tracking, if we ever want to do this sort of thing in target-independent code.

[fhahn]

Add note about relying on the structure of the AArch64 register file as suggested.

[fhahn]

llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp

1296

Thanks Eli, I added a note!

[efriedma]

LGTM with one minor question.

I think the way we choose rename registers prevents using a register that is defined by the same instruction we are renaming. I can see if I can add a test case for that.

Did you ever come up with a testcase for this?

[fhahn]

In D81108#2077860, @efriedma wrote:

LGTM with one minor question.

Thanks Eli!

I think the way we choose rename registers prevents using a register that is defined by the same instruction we are renaming. I can see if I can add a test case for that.

Did you ever come up with a testcase for this?

I pushed a test with an LDP with an undef operand in 22c2dc5931a73eebf2a2e2b2d7a8cf757907dc3e. The undef register would be the next one to pick, but it is skipped (because it is also a def). Was that what you had in mind?

[efriedma]

I pushed a test with an LDP with an undef operand in 22c2dc5931a73eebf2a2e2b2d7a8cf757907dc3e. The undef register would be the next one to pick, but it is skipped (because it is also a def). Was that what you had in mind?

Yes; thanks.

llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp

if (!getLdStRegOp(FirstMI).isKill() &&
               [TRI, RegToRename](const MachineOperand &MOP) {
                 return MOP.isReg() && !MOP.isDebug() && MOP.getReg() &&
                        MOP.isImplicit() && MOP.isKill() &&
                        TRI->regsOverlap(RegToRename, MOP.getReg());
               })) {
     LLVM_DEBUG(dbgs() << "  Operand not killed at " << FirstMI << "\n");
     return false;
   }
-  auto canRenameMOP = [](const MachineOperand &MOP) {
+  auto canRenameMOP = [TRI](const MachineOperand &MOP) {
+    if (MOP.isReg()) {
+      auto *RegClass = TRI->getMinimalPhysRegClass(MOP.getReg());
+      // Renaming registers with multiple disjunct sub-registers (e.g. the
+      // result of a LD3) means that all sub-registers are renamed, potentially
+      // impacting other instructions we did not check. Bail out.
+      // Note that this relies on the structure of the AArch64 register file. In
+      // particular, a subregister cannot be written without overwriting the
+      // whole register.
+      if (RegClass->HasDisjunctSubRegs) {
+        LLVM_DEBUG(
+            dbgs()
+            << "  Cannot rename operands with multiple disjunct subregisters ("
+            << MOP << ")\n");
+        return false;
+      }
+    }
     return MOP.isImplicit() ||
            (MOP.isRenamable() && !MOP.isEarlyClobber() && !MOP.isTied());
   };
 
   bool FoundDef = false;
 
   // For each instruction between FirstMI and the previous def for RegToRename,
   // we

llvm/test/CodeGen/AArch64/stp-opt-with-renaming-ld3.mir

-# XFAIL: *
-# RUN: llc -run-pass=aarch64-ldst-opt -mtriple=arm64-apple-iphoneos -aarch64-load-store-renaming=true -verify-machineinstrs -o - %s | FileCheck %s
+# RUN: llc -run-pass=aarch64-ldst-opt -mtriple=arm64-apple-iphoneos -aarch64-load-store-renaming=true -o - -verify-machineinstrs %s | FileCheck %s
 --- |
   define void @test_ld3(<8 x i8>* %a1) {
   entry:
     %s1 = alloca i64
     ret void
   }
 
 ...
 ---
 # CHECK-LABEL: name: test_ld3
 # CHECK: bb.0.entry:
-# CHECK:       renamable $x0, $d1_d2_d3 = LD3Threev8b_POST killed renamable $x0, $xzr
+# CHECK:       renamable $x0, renamable $d0_d1_d2 = LD3Threev8b_POST killed renamable $x0, $xzr
 # CHECK-NEXT:  STPDi renamable $d0, renamable $d1, $fp, -6 :: (store 8)
+# CHECK-NEXT:  STURDi renamable $d2, $fp, -32, implicit killed $d0_d1_d2 :: (store 8 into %ir.s1)
 # CHECK-NEXT:  renamable $d0_d1_d2 = LD3Threev8b killed renamable $x0 :: (load 24 from %ir.a1, align 32)
-# CHECK-NEXT:  STPDi $d3, renamable $d0, $fp, -4 :: (store 8 into %ir.s1), (store 8)
-# CHECK-NEXT:  STPDi renamable $d1, renamable $d2, $fp, -2 :: (store 8)
+# CHECK-NEXT:  STPDi renamable $d0, renamable $d1, $fp, -3 :: (store 8)
+# CHECK-NEXT:  STURDi renamable $d2, $fp, -8, implicit killed $d0_d1_d2 :: (store 8)
 # CHECK-NEXT:  RET undef $lr
 #
 name:            test_ld3
 alignment:       4
 tracksRegLiveness: true
 frameInfo: {}
 machineFunctionInfo: {}
 body:             |