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

[Xiang Zhang via Phabricator via llvm-commits]

xiangzhangllvm abandoned this revision.
xiangzhangllvm added inline comments.


================
Comment at: llvm/lib/CodeGen/MachineCopyPropagation.cpp:555
+  // renamable $rcx = COPY killed renamable $k0
+  // renamable $k1 = COPY renamable $ecx
+  Register CopyDstReg = CopyOperands->Destination->getReg();
----------------
xiangzhangllvm wrote:
> 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)
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.


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

https://reviews.llvm.org/D147031