[TwoAddressInstruction] Fix ReplacedAllUntiedUses in processTiedPairs

[foad]

Summary

Fix the calculation of ReplacedAllUntiedUses when any of the tied defs
are early-clobber. The effect of this is to fix the placement of kill
flags on an instruction like this (from @f2 in
test/CodeGen/SystemZ/asm-18.ll):

INLINEASM &"stepb $1, $2" [attdialect], $0:[regdef-ec:GRH32Bit], def early-clobber %3:grh32bit, $1:[reguse tiedto:$0], killed %4:grh32bit(tied-def 3), $2:[reguse:GRH32Bit], %4:grh32bit

After TwoAddressInstruction without this patch:

%3:grh32bit = COPY killed %4:grh32bit
INLINEASM &"stepb $1, $2" [attdialect], $0:[regdef-ec:GRH32Bit], def early-clobber %3:grh32bit, $1:[reguse tiedto:$0], %3:grh32bit(tied-def 3), $2:[reguse:GRH32Bit], %4:grh32bit

Note that the COPY kills %4, even though there is a later use of %4 in
the INLINEASM. This fails machine verification if you force it to run
after TwoAddressInstruction (currently it is disabled for other
reasons).

After TwoAddressInstruction with this patch:

%3:grh32bit = COPY %4:grh32bit
INLINEASM &"stepb $1, $2" [attdialect], $0:[regdef-ec:GRH32Bit], def early-clobber %3:grh32bit, $1:[reguse tiedto:$0], %3:grh32bit(tied-def 3), $2:[reguse:GRH32Bit], %4:grh32bit

[MatzeB]

Could you add a .mir test?

[foad]

Rebase on a test case that I will pre-commit.

[MatzeB]

Have you considered .mir tests instead of .ll tests? They should be more robust as they allow you to test a single pass independently of anything happening before/afterwards. See D36224 or test/CodeGen/PowerPC/two-address-crash.mir for examples. There is also llvm/utils/update_mir_test_checks.py to autogenerate the CHECK lines.

[foad]

In D110848#3040368, @MatzeB wrote:

Have you considered .mir tests instead of .ll tests? They should be more robust as they allow you to test a single pass independently of anything happening before/afterwards.

Yes I considered it, but I think there are advantages to both approaches. The problem with a MIR test case is that if the target changes its instruction definitions in any way, updating the MIR input is definitely a manual process.

That said, I'm happy to do a MIR test instead if you still prefer it.

[MatzeB]

Yes I considered it, but I think there are advantages to both approaches. The problem with a MIR test case is that if the target changes its instruction definitions in any way, updating the MIR input is definitely a manual process.

That said, I'm happy to do a MIR test instead if you still prefer it.

The other side of the medal is that the test now depends on the all the passes before to behave the same, so changes to instruction selection, calling convention modeling etc. throw off the tests.

While I may be biased here as to what is more common, I think having a unit test as targetted as possible (1 pass only) is a good principle to aim for.

[MatzeB]

LGTM

[bjope]

In D110848#3040584, @MatzeB wrote:

Yes I considered it, but I think there are advantages to both approaches. The problem with a MIR test case is that if the target changes its instruction definitions in any way, updating the MIR input is definitely a manual process.

That said, I'm happy to do a MIR test instead if you still prefer it.

The other side of the medal is that the test now depends on the all the passes before to behave the same, so changes to instruction selection, calling convention modeling etc. throw off the tests.

While I may be biased here as to what is more common, I think having a unit test as targetted as possible (1 pass only) is a good principle to aim for.

In this case there aren't that many target instructions. All I see in the CHECK:s is COPY and INLINEASM. And maybe it can be reduced even further if using a MIR input?
(well, I won't stop this as is, just thinking that the reasons for not using MIR in this particular case seemed pretty weak)

Btw, the fix looks fine!

[foad]

Switch to MIR test case.

[bjope]

Thank! LGTM!

[foad]

In D110848#3040977, @bjope wrote:

(well, I won't stop this as is, just thinking that the reasons for not using MIR in this particular case seemed pretty weak)

Agreed!

Btw, the fix looks fine!

Thanks!

llvm/lib/CodeGen/TwoAddressInstructionPass.cpp

#endif
     // whether the dest oper writes a subreg, the source oper should not.
     MO.setSubReg(0);
 
     // Propagate SrcRegMap.
     SrcRegMap[RegA] = RegB;
   }
 
   if (AllUsesCopied) {
-    bool ReplacedAllUntiedUses = true;
+    bool ReplacedAllUntiedUses = false;
     if (!IsEarlyClobber) {
       // Replace other (un-tied) uses of regB with LastCopiedReg.
+      ReplacedAllUntiedUses = true;
       for (MachineOperand &MO : MI->operands()) {
         if (MO.isReg() && MO.getReg() == RegB && MO.isUse()) {
           if (MO.getSubReg() == SubRegB) {
             if (MO.isKill()) {
               MO.setIsKill(false);
               RemovedKillFlag = true;
             }
             MO.setReg(LastCopiedReg);

llvm/test/CodeGen/SystemZ/twoaddr-kill.ll

 ; NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
 ; RUN: llc < %s -verify-machineinstrs -mtriple=s390x-linux-gnu -mcpu=z196 -stop-after=twoaddressinstruction -verify-machineinstrs | FileCheck %s
 
-; FIXME: The COPY from [[COPY3]] is killed even though there is a subsequent use
-; of [[COPY3]] in the INLINEASM instruction.
+; Check that the COPY from [[COPY3]] is not killed because there is a subsequent
+; use of [[COPY3]] in the INLINEASM instruction.
 define i32 @f2(i32 %old) {
   ; CHECK-LABEL: name: f2
   ; CHECK: bb.0 (%ir-block.0):
   ; CHECK-NEXT:   liveins: $r2l
   ; CHECK-NEXT: {{  $}}
   ; CHECK-NEXT:   [[COPY:%[0-9]+]]:gr32bit = COPY killed $r2l
   ; CHECK-NEXT:   [[COPY1:%[0-9]+]]:grh32bit = COPY [[COPY]]
   ; CHECK-NEXT:   $r2l = COPY [[COPY]]
   ; CHECK-NEXT:   $r3l = COPY killed [[COPY]]
   ; CHECK-NEXT:   [[COPY2:%[0-9]+]]:grh32bit = COPY killed [[COPY1]]
   ; CHECK-NEXT:   INLINEASM &"stepa $1, $2, $3", 0 /* attdialect */, 393226 /* regdef:GRH32Bit */, def [[COPY2]], 2147483657 /* reguse tiedto:$0 */, [[COPY2]](tied-def 3), 9 /* reguse */, killed $r2l, 9 /* reguse */, killed $r3l
   ; CHECK-NEXT:   [[COPY3:%[0-9]+]]:grh32bit = COPY killed [[COPY2]]
-  ; CHECK-NEXT:   [[COPY4:%[0-9]+]]:grh32bit = COPY killed [[COPY3]]
+  ; CHECK-NEXT:   [[COPY4:%[0-9]+]]:grh32bit = COPY [[COPY3]]
   ; CHECK-NEXT:   INLINEASM &"stepb $1, $2", 0 /* attdialect */, 393227 /* regdef-ec:GRH32Bit */, def early-clobber [[COPY4]], 2147483657 /* reguse tiedto:$0 */, [[COPY4]](tied-def 3), 393225 /* reguse:GRH32Bit */, [[COPY3]]
   ; CHECK-NEXT:   $r2l = COPY killed [[COPY4]]
   ; CHECK-NEXT:   Return implicit killed $r2l
   %tmp = call i32 asm "stepa $1, $2, $3",
                       "=h,0,{r2},{r3}"(i32 %old, i32 %old, i32 %old)
   %new = call i32 asm "stepb $1, $2", "=&h,0,h"(i32 %tmp, i32 %tmp)
   ret i32 %new
 }