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

[Xiang Zhang via Phabricator via llvm-commits]

xiangzhangllvm added inline comments.


================
Comment at: llvm/lib/CodeGen/MachineCopyPropagation.cpp:653
+      ForwardedReg = TRI->getSubReg(CopySrcReg, SubRegIdx);
+      if (!ForwardedReg) {
+        LLVM_DEBUG(dbgs() << "MCP: Copy source does not have sub-register "
----------------
barannikov88 wrote:
> xiangzhangllvm wrote:
> > 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 
> 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.
> 
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.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146930