[TwoAddressInstructionPass] Replace subregister uses when processing tied operands

[bjope]

Summary

TwoAddressInstruction pass typically rewrites

%1:short = foo %0.sub_lo:long

as

%1:short = COPY %0.sub_lo:long
%1:short = foo %1:short

when having tied operands.

If there are extra un-tied operands that uses the same reg and
subreg, such as the second and third inputs to fie here:

%1:short = fie %0.sub_lo:long, %0.sub_hi:long, %0.sub_lo:long

then there was a bug which replaced the register %0 also for
the un-tied operand, but without changing the subregister indices.
So we used to get:

%1:short = COPY %0.sub_lo:long
%1:short = fie %1, %1.sub_hi:short, %1.sub_lo:short

With this fix we instead get:

%1:short = COPY %0.sub_lo:long
%1:short = fie %1, %0.sub_hi:long, %1

[kparzysz]

Please upload diffs with more context in the future (the entire files).

If the instruction had vreg1:lo16 instead, would this patch would still replace it with vreg3?

[kparzysz]

In D36224#829201, @kparzysz wrote:

Please upload diffs with more context in the future (the entire files).

My fault, I somehow didn't notice that it's there... Disregard.

[JesperAntonsson]

Krzysztof, thanks, I think it actually would replace it (though it would have before my patch too). I'll upload a patch that'll prevent it.

[JesperAntonsson]

Made sure that we don't replace register if the subregister is different.

[JesperAntonsson]

Hmm, sorry, I just realized that the check I introduced in the second patch made it unnecessary to set the subregister conditionally, it could just as well be set unconditionally to zero. So I'm doing this in this update.

This means my patch is now reverting the aforementioned patch 279804 by Matt, except for the tests. However, the tests seem to pass even with my revert. So I'm actually a bit confused here, even though I think my patch is correct. I hope Matt can weigh in on this.

[bjope]

I'll take over this, and add a reproducer for an in-tree target!

[bjope]

Addning a hand-crafted reproducer for Hexagon.

[arsenm]

The tests may pass, but is the output exactly the same? I think this should break the test in the listed example. There can't be a copy in the case it's trying to fix

[bjope]

In D36224#1243333, @arsenm wrote:

The tests may pass, but is the output exactly the same? I think this should break the test in the listed example. There can't be a copy in the case it's trying to fix

Sorry, I don't think I understand what you mean. Are you talking about the example in the summary, or the added mir-test?

[arsenm]

In D36224#1243458, @bjope wrote:

In D36224#1243333, @arsenm wrote:

The tests may pass, but is the output exactly the same? I think this should break the test in the listed example. There can't be a copy in the case it's trying to fix

Sorry, I don't think I understand what you mean. Are you talking about the example in the summary, or the added mir-test?

The tests I committed with that revision

[bjope]

In D36224#1245944, @arsenm wrote:

In D36224#1243458, @bjope wrote:

In D36224#1243333, @arsenm wrote:

The tests may pass, but is the output exactly the same? I think this should break the test in the listed example. There can't be a copy in the case it's trying to fix

Sorry, I don't think I understand what you mean. Are you talking about the example in the summary, or the added mir-test?

The tests I committed with that revision

Oh, I see, you answered the question that was 13 months old :-)

Well, your old fix from here https://reviews.llvm.org/rL279804 touched two test cases:

test/CodeGen/AMDGPU/indirect-addressing-si.ll
test/CodeGen/AMDGPU/indirect-addressing-si-noopt.ll

I've been running all RUN-lines from both of those test cases, with -print-after-all, and compared the result on trunk with and without this patch. The result is identical!
So maybe there have been some other fix that has solved the same problem you fixed in rL279804 in some other way (or something that hides it).

We have had the patch from this review (revert of rL279804) active in our out-of-tree target ever since rL279804 started to break our testing.
And I think that given the test case added in this review it is quite obvious that the code on trunk is doing the wrong thing right now. This patch seems to fix the problem.

I'm actually not sure if this should impact the update of liveness info. LV is deprecated and should not be used after this pass (so I don't know if it really need to be updated). And everything about LIS here is unused code unless using -early-live-intervals. And we do not have any tests for that (and when I tried -early-live-intervals I got lots of other failures). Anyway, since rL279804 did not change anything about liveness info, I guess a revert of rL279804 should not impact it either.

[bjope]

In D36224#1246786, @bjope wrote:

I'm actually not sure if this should impact the update of liveness info. LV is deprecated and should not be used after this pass (so I don't know if it really need to be updated). And everything about LIS here is unused code unless using -early-live-intervals. And we do not have any tests for that (and when I tried -early-live-intervals I got lots of other failures). Anyway, since rL279804 did not change anything about liveness info, I guess a revert of rL279804 should not impact it either.

I've looked a little bit more at this and if I add "-run-pass livevars" to make LiveVariables come into play, and also add "-verify-machineinstrs" then the result for "test2" will be:

# After Two-Address instruction pass
# Machine code for function test2: NoPHIs, TracksLiveness

bb.0.entry:
  liveins: $d0
  %0:doubleregs = COPY killed $d0
  %1:intregs = COPY killed %0.isub_lo:doubleregs
  dead %1:intregs = S2_lsr_i_r_acc %1:intregs(tied-def 0), %0.isub_hi:doubleregs, 16

# End machine code for function test2.

*** Bad machine code: Using a killed virtual register ***
- function:    test2
- basic block: %bb.0 entry (0x5c773b8)
- instruction: dead %1:intregs = S2_lsr_i_r_acc %1:intregs(tied-def 0), %0.isub_hi:doubleregs, 16
- operand 2:   %0.isub_hi:doubleregs
LLVM ERROR: Found 1 machine code errors.

I've been told that "dead" markings on subreg defs only tell that the subreg is dead, not the full register. Is it the same for "killed"?
If so, then I'd assume that this is a verifier bug. Otherwise we need to remove killed %0.isub_lo from the COPY instruction here.

PS. Another thing is that we do not get killed on the %0.isub_hi use in the S2_lsr_i_r_acc in the result. But we do not even get that out from LiveVariables, so maybe we should not blame TwoAddressInstructionPass for that.

[MatzeB]

Note that embarassingly there are still a few passes where we skip machine verification in the default pass pipeline, TwoAddressInstruction being one of them. However if this is a new failure triggered by this particular patch then please fix it!

In D36224#1247994, @bjope wrote:

In D36224#1246786, @bjope wrote:

I'm actually not sure if this should impact the update of liveness info. LV is deprecated and should not be used after this pass (so I don't know if it really need to be updated). And everything about LIS here is unused code unless using -early-live-intervals. And we do not have any tests for that (and when I tried -early-live-intervals I got lots of other failures). Anyway, since rL279804 did not change anything about liveness info, I guess a revert of rL279804 should not impact it either.

I've looked a little bit more at this and if I add "-run-pass livevars" to make LiveVariables come into play, and also add "-verify-machineinstrs" then the result for "test2" will be:

# After Two-Address instruction pass
# Machine code for function test2: NoPHIs, TracksLiveness

bb.0.entry:
  liveins: $d0
  %0:doubleregs = COPY killed $d0
  %1:intregs = COPY killed %0.isub_lo:doubleregs
  dead %1:intregs = S2_lsr_i_r_acc %1:intregs(tied-def 0), %0.isub_hi:doubleregs, 16

# End machine code for function test2.

*** Bad machine code: Using a killed virtual register ***
- function:    test2
- basic block: %bb.0 entry (0x5c773b8)
- instruction: dead %1:intregs = S2_lsr_i_r_acc %1:intregs(tied-def 0), %0.isub_hi:doubleregs, 16
- operand 2:   %0.isub_hi:doubleregs
LLVM ERROR: Found 1 machine code errors.

I've been told that "dead" markings on subreg defs only tell that the subreg is dead, not the full register. Is it the same for "killed"?

No! dead, killed and undef are specifically not about the subregister part but about the full vreg. (In an ideal world for subregister liveness we would have a lanemask for dead,killed,undef instead of just the bit we have today. With only a bit available we mostly ignore them for subreg liveness purposes as we cannot express situations like a write with only some lanes dead anyway...)

If so, then I'd assume that this is a verifier bug. Otherwise we need to remove killed %0.isub_lo from the COPY instruction here.

this is bad MI here.

PS. Another thing is that we do not get killed on the %0.isub_hi use in the S2_lsr_i_r_acc in the result. But we do not even get that out from LiveVariables, so maybe we should not blame TwoAddressInstructionPass for that.

yes as mentioned above, there may be bad pre-existing things hiding here :-(

[bjope]

1. Added livevars and verify-machineinstrs to the RUN line in the test case.
2. Solved the fault detected by the verifier, i.e. incorrectly setting of killed on %0 in the inserted COPY instruction in test2.

[bjope]

In D36224#1248159, @MatzeB wrote:

Note that embarassingly there are still a few passes where we skip machine verification in the default pass pipeline, TwoAddressInstruction being one of them. However if this is a new failure triggered by this particular patch then please fix it!

My first fix was to avoid replacing untied operands if the untied operands did not use the same subreg as the tied operands.
The thing I originally missed was that when not replacing all uses, it was incorrect to add a kill flag on the use in the inserted COPY. That problem is solved in the latest update of this patch (from September 28), by only adding the kill if we ReplacedAllUntiedUses.

[bjope]

ping!

[MatzeB]

LGTM

lib/CodeGen/TwoAddressInstructionPass.cpp

if (!IsEarlyClobber) {
       for (MachineOperand &MO : MI->operands()) {
         if (MO.isReg() && MO.getReg() == RegB &&
             MO.isUse()) {
           if (MO.isKill()) {
             MO.setIsKill(false);
             RemovedKillFlag = true;
           }
           MO.setReg(LastCopiedReg);
-          MO.setSubReg(MO.getSubReg());
+          if (SubRegB)
+            MO.setSubReg(0);
         }
       }
     }
 
     // Update live variables for regB.
     if (RemovedKillFlag && LV && LV->getVarInfo(RegB).removeKill(*MI)) {
       MachineBasicBlock::iterator PrevMI = MI;
       --PrevMI;