[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 "
----------------
xiangzhangllvm wrote:
> 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.
> 
@barannikov88 I enhance it at [[ https://reviews.llvm.org/D147031 | D147031]]
Pls help review.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146930