[PATCH] D147031: [MachineCopy] Enhance sub register machine copy propagation

[Xiang Zhang via Phabricator via llvm-commits]

xiangzhangllvm added inline comments.


================
Comment at: llvm/lib/CodeGen/MachineCopyPropagation.cpp:427-433
+bool MachineCopyPropagation::hasSameRegClass(Register Def, Register Src) {
+  for (const TargetRegisterClass *RC : TRI->regclasses()) {
+    if (RC->contains(Def) && RC->contains(Src))
+      return true;
+  }
+  return false;
+}
----------------
arsenm wrote:
> You should be able to use existing, generated class relation queries 
Can you gave more hint pls ?
I see the existing code use same way to compare RegClass, pls refer lambda 
 CheckCopyConstraint


================
Comment at: llvm/lib/CodeGen/MachineCopyPropagation.cpp:555
+  // renamable $rcx = COPY killed renamable $k0
+  // renamable $k1 = COPY renamable $ecx
+  Register CopyDstReg = CopyOperands->Destination->getReg();
----------------
barannikov88 wrote:
> 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.
> 
Yes, after you commit [[ https://reviews.llvm.org/D146930 | 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  [[ https://reviews.llvm.org/D146930 | 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)


================
Comment at: llvm/lib/CodeGen/MachineCopyPropagation.cpp:558-560
+  if ((TRI->getRegSizeInBits(CopyDstReg, *MRI) !=
+       TRI->getRegSizeInBits(UseSrcReg, *MRI)) &&
+      !hasSameRegClass(CopyDstReg, CopySrcReg))
----------------
arsenm wrote:
> Shouldn't need to operate in terms of register sizes
Yes, let me directly check SubReg relation here.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147031/new/

https://reviews.llvm.org/D147031