[PATCH] D98659: [MachineCopyPropagation] Do more backward copy propagations

[yshui via Phabricator via llvm-commits]

yshui added a comment.

In D98659#2830241 <https://reviews.llvm.org/D98659#2830241>, @jmorse wrote:

> Looks like the verifier error I experienced must have been transient / my fault, I can't reproduce it now,
>
> I think I understand what's going on here now, however it involves re-purposing the "Avail" flag attached to copies in an unexpected way. At the very least, it needs re-naming. As far as I understand it before this patch, avail means:
>
> - False: candidate for def being rewritten in backward copy propagation
> - False: is a clobbered register, in forward copy propagation
> - True: candidate for forward copy propagation
>
> With this patch the "false" value takes on even more meaning: "uses can be rewritten along the way". Realistically, I think if this is to all be understood, the "Avail" flag needs to become an enum or something to communicate more information to the reader about what it means.

I think it's just "Avail" has different meaning in forward/backward propagation. In backward propagation, it means "def is a candidate for rewriting", this patch doesn't add a new meaning.

> How does this new form of backwards copy propagation interact with the existing backwards copy propagation? The reason being, modifying one your tests a little:
>
>     ---
>
>   name: copyprop3
>   body: |
>     bb.0:
>       renamable $rbp = LEA64r $rax, 1, $noreg, 8, $noreg
>       NOOP implicit renamable $rbp
>       renamable $rax = COPY renamable $rbp
>       renamable $rbx = COPY killed renamable $rbp
>       NOOP implicit $rax
>   ...
>
> I was expecting $rbp to be rewritten to $rax and the copies dropped -- instead the dead COPY is dropped, and the uses left un-rewritten:
>
>   bb.0:
>     renamable $rbp = LEA64r $rax, 1, $noreg, 8, $noreg
>     NOOP implicit renamable $rbp
>     renamable $rax = COPY renamable $rbp
>     NOOP implicit $rax
>
> I suspect (90% confidence) that this is because CopyTracker::invalidateRegister is deleting the copy because it's not eligible for the "plain" backwards copy propagation. The design of these two things needs to be unified.

`renamable $rax = COPY renamable $rbp` isn't a candidate for propagation because `$rbp` isn't killed in that instruction. The later copy is probably eliminated because it's dead code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98659