In D156817#4620998, @mstorsjo wrote:

I noticed this review is accepted but not committed. If you don't have commit access, what's your preferred git identity, i.e. Real Name <email@address>?

In D156817#4553045, @DavidSpickett wrote:

What exactly are the other bits in the mode here, are we losing something important potentially? I guess it can't be that important if Windows rejects them.

Rebasing, new affected test:

In 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.

Fix a missing period

One more affected test: CodeGen/ARM/umulo-128-legalisation-lowering.ll

@jmorse Ping.

Rebasing, no new affected tests.

• Rebasing
• More affected tests:
  • CodeGen/RISCV/rvv/vselect-fp-rv64.ll
  • CodeGen/RISCV/rvv/vselect-fp-rv32.ll

Rebasing

• Rebase
• Add an example of the new copy propagation in the top comments
• One more updated test cases: CodeGen/X86/vector-interleaved-load-i16-stride-4.ll
• Style fixes

Add tests suggested by @jmorse

In D98659#2661440, @jmorse wrote:

The simplest and easiest fix for that, is to have the function ignore debug-info instructions by adding a "if (MI.isDebugInstr()) continue;" statement in the loop before interpreting its register-reads.

Uh, I just figured out how mir tests work. Guess nobody wanted to point out how dumb I am. 😅

Rebasing

Thanks @jmorse for the review.

In D98659#2648161, @jmorse wrote:

There isn't a lot of difference to how debug-info is updated, as far as I can see, but the summary says a fault is resolved. Is this patch just masking the debug-info issue by doing more propagation? (I might have missed something in the implementation).

Style fixes.

In D98659#2634206, @lkail wrote:

Thanks for your review. There is a test case in https://reviews.llvm.org/D98401, which is quite big. I wasn't able to reduce it further, is it OK to just include that as test case?

Have you tried using bugpoint to reduce it further, or writing MIR test manually?

In D98659#2633657, @lkail wrote:

In D98659#2632859, @yshui wrote:

Since this patch already changed output of many test cases, do we still need a test case specifically for this patch?

You'd better add some mir tests to demonstrate the idea, w/o -g.

Since this patch already changed output of many test cases, do we still need a test case specifically for this patch?

Update Mips tests.

Include full context in the patch

I reviewed the changes in CodeGen tests to the best of my ability, and they look fine to me. If someone else could review and confirm, that would be appreciated.

Strengthen the invariant maintained by the CopyTracker.

https://reviews.llvm.org/D98659 could supersede this

Adjust a debug message.

Remove a TODO which is no longer relevant

Adjust how DBG_VALUE is rewritten.

Fix typos in comments.

Things still need to be done:

I think it might be valuable to extend this to all cases like this:

Because you replaced $rbp with $rbx, surely you would have to replace it everywhere. Removing the DBG_VALUE is potential information loss.