[PATCH] D157564: [MCP] Invalidate copy for super register in copy source

[Jeffrey Byrnes via Phabricator via llvm-commits]

jrbyrnes added a comment.

In D157564#4575259 <https://reviews.llvm.org/D157564#4575259>, @lkail wrote:

>   5: USE r6
>   6: r1:4 = COPY r6:9
>
> Hi @jrbyrnes , do you know why MCP fails to invalidate `r7:r9` at label 5? In my view, when we are invalidating `r6` at label 5, we should also find the copy involving def or use of `r6` which is label 6, and invalidate the regunits of both src operands(`r6:r9`) and dest operands(`r1:r4`), which is what `invalidateRegister` doing.
>
>             RegsToInvalidate.insert(
>                 CopyOperands->Destination->getReg().asMCReg());
>             RegsToInvalidate.insert(CopyOperands->Source->getReg().asMCReg());
>   ...
>       for (MCRegister InvalidReg : RegsToInvalidate)
>         for (MCRegUnit Unit : TRI.regunits(InvalidReg))
>           Copies.erase(Unit);

We only insert the MI corresponding to the copies for tracking the defs

  for (MCRegUnit Unit : TRI.regunits(Def))
    Copies[Unit] = {MI, nullptr, {}, {}, true};
  
  // Remember source that's copied to Def. Once it's clobbered, then
  // it's no longer available for copy propagation.
  for (MCRegUnit Unit : TRI.regunits(Src)) {
    auto I = Copies.insert({Unit, {nullptr, nullptr, {}, {}, false}});
    auto &Copy = I.first->second;
    if (!is_contained(Copy.DefRegs, Def))
      Copy.DefRegs.push_back(Def);
    if (!is_contained(Copy.SrcRegs, Src))
      Copy.SrcRegs.push_back(Src);
    Copy.LastSeenUseInCopy = MI;
  }

Then, when calling `invalidateRegister` for the `r6` in label 5, it is unable to find an MI in the `CopyTracker`. Thus

  if (MachineInstr *MI = I->second.MI) {
    std::optional<DestSourcePair> CopyOperands =
        isCopyInstr(*MI, TII, UseCopyInstr);
    assert(CopyOperands && "Expect copy");
  
    RegsToInvalidate.insert(
        CopyOperands->Destination->getReg().asMCReg());
    RegsToInvalidate.insert(CopyOperands->Source->getReg().asMCReg());
  }

is never reached. I don't claim to understand why it was designed this way, but, from the perspective of this example, it seems that we should also be tracking this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157564