This is an archive of the discontinued LLVM Phabricator instance.

[MachineCopy] Enhance sub register machine copy propagation
AbandonedPublic

Authored by xiangzhangllvm on Mar 28 2023, 1:05 AM.

Details

Summary

The machine copy propagation logic has some problem:
When do machine copy propagation for subreg we need to consider the CopyDstReg and CopySrcReg's regclass and size.
(because we want to get SubReg from CopySrcReg by using SubregIdx which we got from CopyDstReg)
Take follow case for example:

MCP: Replacing $ecx
     with $k0
     in renamable $k1 = COPY renamable $ecx
     from renamable $rcx = COPY renamable $k0, implicit-def $ecx

We can not use SubregIdx (ecx of rcx) to get anything from k0 register.

The upper test case problem was fix/escaped by D146930
But there is still risk in it. More details PLS refer to comments in D146930
Now this patch is to enhance the safety in propagating SubReg.

Diff Detail

Event Timeline

xiangzhangllvm created this revision.Mar 28 2023, 1:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 28 2023, 1:05 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
xiangzhangllvm requested review of this revision.Mar 28 2023, 1:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 28 2023, 1:05 AM

Does it try to solve the same problem as D146930?

Does it try to solve the same problem as D146930?

Very similar from the test case, I commented on D146930

It crashes on the test from D146930.

barannikov88 added inline comments.Mar 28 2023, 5:27 PM
llvm/lib/CodeGen/MachineCopyPropagation.cpp
672

All registers in a register class have the same sizes. The check for size above is redundant.

lkail added a subscriber: lkail.Mar 28 2023, 5:43 PM
lkail added inline comments.
llvm/lib/CodeGen/MachineCopyPropagation.cpp
631

This looks similar to what isForwardableRegClassCopy does? Can we extend isForwardableRegClassCopy to contain subregindex?

It crashes on the test from D146930.

Sorry for late respond. Let me take a look on your test.
Anyway this bug affect our critical tests,I +1 for let D146930 first in.
I also comment the risk in D146930.

llvm/lib/CodeGen/MachineCopyPropagation.cpp
631

Sound good, we can check it in that function.

672

Make sense!

xiangzhangllvm added inline comments.Mar 29 2023, 7:32 PM
llvm/lib/CodeGen/MachineCopyPropagation.cpp
631

isForwardableRegClassCopy already try handle cross-class case.
Looks we only need to enhance it:

diff --git a/llvm/lib/CodeGen/MachineCopyPropagation.cpp b/llvm/lib/CodeGen/MachineCopyPropagation.cpp
index 218f293a4085..fff2893e2148 100644
--- a/llvm/lib/CodeGen/MachineCopyPropagation.cpp
+++ b/llvm/lib/CodeGen/MachineCopyPropagation.cpp
@@ -539,6 +539,16 @@ bool MachineCopyPropagation::isForwardableRegClassCopy(const MachineInstr &Copy,
   /// so we have reduced the number of cross-class COPYs and potentially
   /// introduced a nop COPY that can be removed.

+  // We don't propagate for SubReg in cross-class COPYs.
+  // For example: (ecx is SubReg of rcx)
+  // renamable $rcx = COPY killed renamable $k0
+  // renamable $k1 = COPY renamable $ecx
+  Register CopyDstReg = CopyOperands->Destination->getReg();
+  Register UseSrcReg = UseICopyOperands->Source->getReg();
+  if (TRI->getRegSizeInBits(CopyDstReg, *MRI) !=
+      TRI->getRegSizeInBits(UseSrcReg, *MRI))
+    return false;
+
   // Allow forwarding if src and dst belong to any common class, so long as they
   // don't belong to any (possibly smaller) common class that requires copies to
   // go via a different class.

Let me do more testing, thanks!

xiangzhangllvm edited the summary of this revision. (Show Details)
xiangzhangllvm retitled this revision from [MachineCopy] Bug fix sub register machine copy propagation to [MachineCopy] Enhance sub register machine copy propagation.
barannikov88 added inline comments.Mar 30 2023, 2:01 AM
llvm/lib/CodeGen/MachineCopyPropagation.cpp
555

This method is not called for this example (look for "MCP: Copy source does not have sub-register" below).
$ecx is a sub-register of $rcx, and its sub-register index is sub_32bit. $k0 does not have sub-registers with index sub_32bit.
You can check my assertion by removing this code and making sure that the added test still passes.

arsenm requested changes to this revision.Mar 30 2023, 4:04 AM
arsenm added a subscriber: arsenm.
arsenm added inline comments.
llvm/lib/CodeGen/MachineCopyPropagation.cpp
427–433

You should be able to use existing, generated class relation queries

558–560

Shouldn't need to operate in terms of register sizes

This revision now requires changes to proceed.Mar 30 2023, 4:04 AM
xiangzhangllvm added inline comments.Mar 30 2023, 6:04 PM
llvm/lib/CodeGen/MachineCopyPropagation.cpp
427–433

Can you gave more hint pls ?
I see the existing code use same way to compare RegClass, pls refer lambda
CheckCopyConstraint

555

Yes, after you commit D146930 the old test machine-copy-subreg.mir can pass without this patch. I need to create a new test.

This patch is not handle "MCP: Copy source does not have sub-register". It try to handle "we can get sub-register from copy source, but the sub-register is wrong"

As I comment in D146930
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)

558–560

Yes, let me directly check SubReg relation here.

xiangzhangllvm abandoned this revision.Apr 6 2023, 2:30 AM
xiangzhangllvm added inline comments.
llvm/lib/CodeGen/MachineCopyPropagation.cpp
555

I think the risk is not existed.
Here the unsigned sub-register index imply both the offset and size ( sub_32bit)
Not only index of sub-register in one register 's subs.
Though @barannikov88 mentioned sub_32bit before, but I am not 100% make sure about it.
Let me close this patch.