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

[Sergei Barannikov via Phabricator via llvm-commits]

barannikov88 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 "
----------------
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.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146930