[MCP] Do not try forward non-existent sub-register of a copy

[barannikov88]

Summary

In this example:

$d14 = COPY killed $d18
$s0 = MI $s28

$s28 is a sub-register of $d14. However, $d18 does not have
sub-registers and thus cannot be forwarded. Previously, this resulted
in $noreg being substituted in place of the use of $s28, which later
led to an assertion failure.

Fixes https://github.com/llvm/llvm-project/issues/60908, a regression
that was introduced in D141747.

[barannikov88]

Add a positive test

[barannikov88]

Rebase

[barannikov88]

Strip changes that are not necessary for fixing the bug.
They will be in a separate commit.

[barannikov88]

Drop 'renamable' on the registers that are not really renamable.

[glandium]

I'm not a reviewer, but I can confirm the patch fixes the issue as it was filed.

[arsenm]

llvm/lib/CodeGen/MachineCopyPropagation.cpp

651

Don't really understand the assert message here (plus I thought getSubRegIndex could deal with the 0 case anyway)

[barannikov88]

llvm/lib/CodeGen/MachineCopyPropagation.cpp

651

findAvailCopy returns non-null only if isSubRegisterEq(CopyDstReg, MOUse.getReg()) returns true, i.e. if CopyDstReg is the same as MOUse.getReg() or is a sub-register of MOUse.getReg(). If it is a sub-register, it must have a valid sub-register index, and this is checked by this assertion.
Could you suggest how to rephrase the assert message? My English is bad at times.

[barannikov88]

llvm/lib/CodeGen/MachineCopyPropagation.cpp

651

I messed up operands of isSubRegisterEq. It should read:

"if isSubRegisterEq(CopyDstReg, MOUse.getReg()) returns true, i.e. if MOUse.getReg() is the same as CopyDstReg or is a sub-register of CopyDstReg."

renamable $d14 = COPY killed $d15
$s0 = COPY killed renamable $s28

Here, $d14 is CopyDstReg, $s28 is MOUse.getReg(), and $d15 is CopySrcReg.

[xiangzhangllvm]

llvm/lib/CodeGen/MachineCopyPropagation.cpp

653

I think there is unsafe to use the SubRegIdx when CopySrcReg and CopyDstReg reg are not same regclass or even different size.
I happen to meet this bug in our tests.
Pls refer to https://reviews.llvm.org/D147031

[barannikov88]

llvm/lib/CodeGen/MachineCopyPropagation.cpp

653

Why do you think it is unsafe?

The test contains:

renamable $rcx = COPY killed renamable $k0
renamable $k1 = COPY renamable $ecx

Suppose it is a valid MIR, despite of the fact that $k0, $ecx and $rcx are all of different sizes.

With D147031 it is converted to (according to CHECK lines):

renamable $rcx = COPY renamable $k0
renamable $k1 = COPY $k0

Which, I think is a change of semantics. (What's the semantics of a COPY to a differently-sized register?)

With this patch it remains untouched:

renamable $rcx = COPY killed renamable $k0
renamable $k1 = COPY renamable $ecx

This patch does not try to optimize code, it only fixes the reported issue. But it does not generate invalid code (unless proven otherwise :).

@xiangzhangllvm
If you don't mind, I'd land this patch as it fixes the original issue. It would be nice if you could check it on your codebase though.
Feel free to update your patch if this one wouldn't fix your issue, I'd gladly review it.

[xiangzhangllvm]

llvm/lib/CodeGen/MachineCopyPropagation.cpp

653

1st, sorry for I am off line yesterday (for some family errands).
2nd, I think your patch can also fix my test fail. So I +1 for you to commit this patch first. Any other problems, we can go on refine it.
3rd, For different reg class, the following convert has risk.

B= COPY C
A= COPY SubB

==>
A = COPY SubC

Because the SubRegIdx of B (for SubB) may not match the SubRegIdx of C (for SubC).
(for example B has 2 subreg but C only has 4)

Anyway, this can be refine later. We can let this patch in first.

[barannikov88]

Try to refine assertion message.

[xiangzhangllvm]

llvm/lib/CodeGen/MachineCopyPropagation.cpp

653

@barannikov88 I enhance it at D147031
Pls help review.

llvm/lib/CodeGen/MachineCopyPropagation.cpp

if (!Copy)
       continue;
 
     std::optional<DestSourcePair> CopyOperands =
         isCopyInstr(*Copy, *TII, UseCopyInstr);
     Register CopyDstReg = CopyOperands->Destination->getReg();
     const MachineOperand &CopySrc = *CopyOperands->Source;
     Register CopySrcReg = CopySrc.getReg();
 
-    // When the use is a subregister of the COPY destination,
-    // record the subreg index.
-    unsigned SubregIdx = 0;
-
-    // This can only occur when we are dealing with physical registers.
+    Register ForwardedReg = CopySrcReg;
+    // MI might use a sub-register of the Copy destination, in which case the
+    // forwarded register is the matching sub-register of the Copy source.
     if (MOUse.getReg() != CopyDstReg) {
-      SubregIdx = TRI->getSubRegIndex(CopyDstReg, MOUse.getReg());
-      if (!SubregIdx)
+      unsigned SubRegIdx = TRI->getSubRegIndex(CopyDstReg, MOUse.getReg());
+      assert(SubRegIdx &&
+             "MI source is not a sub-register of Copy destination");
+      ForwardedReg = TRI->getSubReg(CopySrcReg, SubRegIdx);
+      if (!ForwardedReg) {
+        LLVM_DEBUG(dbgs() << "MCP: Copy source does not have sub-register "
+                          << TRI->getSubRegIndexName(SubRegIdx) << '\n');
         continue;
       }
+    }
 
     // Don't forward COPYs of reserved regs unless they are constant.
     if (MRI->isReserved(CopySrcReg) && !MRI->isConstantPhysReg(CopySrcReg))
       continue;
 
     if (!isForwardableRegClassCopy(*Copy, MI, OpIdx))
       continue;
 
for (unsigned OpIdx = 0, OpEnd = MI.getNumOperands(); OpIdx < OpEnd;
 
     if (!DebugCounter::shouldExecute(FwdCounter)) {
       LLVM_DEBUG(dbgs() << "MCP: Skipping forwarding due to debug counter:\n  "
                         << MI);
       continue;
     }
 
     LLVM_DEBUG(dbgs() << "MCP: Replacing " << printReg(MOUse.getReg(), TRI)
-                      << "\n     with " << printReg(CopySrcReg, TRI)
+                      << "\n     with " << printReg(ForwardedReg, TRI)
                       << "\n     in " << MI << "     from " << *Copy);
 
-    if (SubregIdx)
-      MOUse.setReg(TRI->getSubReg(CopySrcReg, SubregIdx));
-    else
-      MOUse.setReg(CopySrcReg);
+    MOUse.setReg(ForwardedReg);
 
     if (!CopySrc.isRenamable())
       MOUse.setIsRenamable(false);
     MOUse.setIsUndef(CopySrc.isUndef());
 
     LLVM_DEBUG(dbgs() << "MCP: After replacement: " << MI << "\n");
 
     // Clear kill markers that may have been invalidated.

llvm/test/CodeGen/ARM/pr60908.mir

+# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py UTC_ARGS: --version 2
+# RUN: llc -mtriple=armv7 -run-pass=machine-cp %s -o - | FileCheck %s
+
+# Positive test: $d15 has sub-register $s30, which should be propagated.
+---
+name:            test_d15
+tracksRegLiveness: true
+liveins:
+  - { reg: '$d15' }
+body:             |
+  bb.0.entry:
+    liveins: $d15
+
+    ; CHECK-LABEL: name: test_d15
+    ; CHECK: liveins: $d15
+    ; CHECK-NEXT: {{  $}}
+    ; CHECK-NEXT: $s0 = COPY $s30
+    ; CHECK-NEXT: BX_RET 14 /* CC::al */, $noreg, implicit killed $s0
+    renamable $d14 = COPY killed $d15
+    $s0 = COPY killed renamable $s28
+    BX_RET 14 /* CC::al */, $noreg, implicit killed $s0
+
+...
+
+# Negative test: $d18 does not have sub-registers.
+---
+name:            test_d18
+tracksRegLiveness: true
+liveins:
+  - { reg: '$d18' }
+body:             |
+  bb.0.entry:
+    liveins: $d18
+
+    ; CHECK-LABEL: name: test_d18
+    ; CHECK: liveins: $d18
+    ; CHECK-NEXT: {{  $}}
+    ; CHECK-NEXT: renamable $d14 = COPY killed $d18
+    ; CHECK-NEXT: $s0 = COPY killed renamable $s28
+    ; CHECK-NEXT: BX_RET 14 /* CC::al */, $noreg, implicit killed $s0
+    renamable $d14 = COPY killed $d18
+    $s0 = COPY killed renamable $s28
+    BX_RET 14 /* CC::al */, $noreg, implicit killed $s0
+
+...