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

[Jeremy Morse via Phabricator via llvm-commits]

jmorse added a comment.

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.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98659