Propagate tied operands when copying a MachineInstr.

[simon_tatham]

Summary

MachineInstr's copy constructor works by calling the addOperand method
to add each operand of the old MachineInstr to the new one, one by
one. But addOperand deliberately avoids trying to replicate ties
between operands, on the grounds that the tie refers to operands by
index, and the indices aren't necessarily finalized yet.

This led to a code generation fault when the machine pipeliner cloned
an Arm conditional instruction, and lost the tie between the output
register and the input value to be used when the condition failed to
execute.

[dmgreen]

This sounds OK to me, and is probably how I would fix this. But it will effect more than just the MachinePipeliner, anything that goes through CloneMachineInstr. Looking at where that is uses, that seems like most the uses should be OK. I'm don't think many backends will use these implicit tied uses, and copying them sounds more correct during a clone.

There are used for inline assembly though. There is a bit of code in the Pipeliner that:

MachineInstr *NewMI = MF.CloneMachineInstr(OldMI);
// Check for tied operands in inline asm instructions. This should be handled
// elsewhere, but I'm not sure of the best solution.
if (OldMI->isInlineAsm()) {
  ... tie operands together

Can this be removed now? I don't see any pipelining + inline assembly tests in the test suite. And with inline asm usually acting as a barrier it wasn't easy for me to make an example. It may be trying to tie operands together twice now.

[arsenm]

Is there a missing verifier check?

[simon_tatham]

In D135434#3845431, @dmgreen wrote:

This sounds OK to me, and is probably how I would fix this. But it will effect more than just the MachinePipeliner, anything that goes through CloneMachineInstr. Looking at where that is uses, that seems like most the uses should be OK. I'm don't think many backends will use these implicit tied uses, and copying them sounds more correct during a clone.

Yes, I think that specifically because this is in the copy constructor, it seems to me that the expectation is that the output MachineInstr looks semantically identical to the input one.

Can this be removed now? I don't see any pipelining + inline assembly tests in the test suite.

I agree that it looks as if that code ought to be redundant now. But I tried removing that code and my fix, and the only test that failed was my own new one. So I also agree that we don't seem to have any tests of it, unfortunately!

In D135434#3845660, @arsenm wrote:

Is there a missing verifier check?

That sounds very plausible. I did try -verify-machineinstrs on the example that originally failed for me, and was a bit surprised that it reported no problems and still generated incorrect output code. So yes, it might well be that someone who knows the MachineVerifier might be able to add a check for ties based on checking the MachineInstr against the MCID. But it's large and has many confusing subfunctions and I'm not familiar enough with it to be able to dash that off quickly, so I hope it need not block fixing the actual correctness issue.

[dmgreen]

In D135434#3846427, @simon_tatham wrote:

In D135434#3845431, @dmgreen wrote:

This sounds OK to me, and is probably how I would fix this. But it will effect more than just the MachinePipeliner, anything that goes through CloneMachineInstr. Looking at where that is uses, that seems like most the uses should be OK. I'm don't think many backends will use these implicit tied uses, and copying them sounds more correct during a clone.

Yes, I think that specifically because this is in the copy constructor, it seems to me that the expectation is that the output MachineInstr looks semantically identical to the input one.

Can this be removed now? I don't see any pipelining + inline assembly tests in the test suite.

I agree that it looks as if that code ought to be redundant now. But I tried removing that code and my fix, and the only test that failed was my own new one. So I also agree that we don't seem to have any tests of it, unfortunately!

I agree that this is an improvement for the copy constructor. I worry that that if that code is left is and there is a case where the inlineasm is copied, we end up trying to copy the tied-operands twice and end up crashing. It seems that the pipeliner is OK with inline-asm in general, so it's probably worth making a test case if we can.

[simon_tatham]

Going by the new test llvm/test/CodeGen/Thumb2/pipeliner-inlineasm, the two pieces of code don't conflict if both are present. But the code in the pipeliner also doesn't seem to be necessary any more. So I've revised this patch to remove it, and do all tie propagation centrally in MachineInstr's copy constructor.

[bcahoon]

I think the changes in this patch, moving the handling to tied operands to CloneMachineInstr and removing the code
from ModuleSchedule.cpp, is a good change. I'm sure there is a test case for this in the Hexagon internal repo. Adding
@sgundapa in case he is able to check? I don't remember why I added this code to MachinePipeliner rather than
change CloneMachineInstr, other than I was trying to quickly fix a pipeliner bug and wasn't sure what effect changing
CloneMachineInstr would cause. But it does make more sense to put it in CloneMachineInstr.

[dmgreen]

Yeah this SGTM. It might have a larger effect than just the MachinePipeliner, but it seems like a better, less error prone default for cloning instructions. Providing you run update_mir_test_checks on the new file, LGTM.

llvm/lib/CodeGen/MachineInstr.cpp

MachineInstr::MachineInstr(MachineFunction &MF, const MachineInstr &MI)
 
   CapOperands = OperandCapacity::get(MI.getNumOperands());
   Operands = MF.allocateOperandArray(CapOperands);
 
   // Copy operands.
   for (const MachineOperand &MO : MI.operands())
     addOperand(MF, MO);
 
+  // Replicate ties between the operands, which addOperand was not
+  // able to do reliably.
+  for (unsigned i = 0, e = getNumOperands(); i < e; ++i) {
+    MachineOperand &NewMO = getOperand(i);
+    const MachineOperand &OrigMO = MI.getOperand(i);
+    NewMO.TiedTo = OrigMO.TiedTo;
+  }
+
   // Copy all the sensible flags.
   setFlags(MI.Flags);
 }
 
 void MachineInstr::moveBefore(MachineInstr *MovePos) {
   MovePos->getParent()->splice(MovePos, getParent(), getIterator());
 }
 

llvm/lib/CodeGen/ModuloSchedule.cpp

 }
 
 /// Clone the instruction for the new pipelined loop and update the
 /// memory operands, if needed.
 MachineInstr *ModuloScheduleExpander::cloneInstr(MachineInstr *OldMI,
                                                  unsigned CurStageNum,
                                                  unsigned InstStageNum) {
   MachineInstr *NewMI = MF.CloneMachineInstr(OldMI);
-  // Check for tied operands in inline asm instructions. This should be handled
-  // elsewhere, but I'm not sure of the best solution.
-  if (OldMI->isInlineAsm())
-    for (unsigned i = 0, e = OldMI->getNumOperands(); i != e; ++i) {
-      const auto &MO = OldMI->getOperand(i);
-      if (MO.isReg() && MO.isUse())
-        break;
-      unsigned UseIdx;
-      if (OldMI->isRegTiedToUseOperand(i, &UseIdx))
-        NewMI->tieOperands(i, UseIdx);
-    }
   updateMemOperands(*NewMI, *OldMI, CurStageNum - InstStageNum);
   return NewMI;
 }
 
 /// Clone the instruction for the new pipelined loop. If needed, this
 /// function updates the instruction using the values saved in the
 /// InstrChanges structure.
 MachineInstr *ModuloScheduleExpander::cloneAndChangeInstr(

llvm/test/CodeGen/Thumb2/pipeliner-inlineasm.mir

body:             |
   ; CHECK-NEXT: {{  $}}
   ; CHECK-NEXT: bb.5.for.body:
   ; CHECK-NEXT:   successors: %bb.6(0x40000000), %bb.9(0x40000000)
   ; CHECK-NEXT: {{  $}}
   ; CHECK-NEXT:   [[t2ADDri:%[0-9]+]]:rgpr = t2ADDri [[COPY4]], 4, 14 /* CC::al */, $noreg, $noreg
   ; CHECK-NEXT:   [[VLDRS2:%[0-9]+]]:spr = VLDRS [[COPY4]], 1, 14 /* CC::al */, $noreg :: (load (s32) from %ir.scevgep7)
   ; CHECK-NEXT:   [[t2ADDri1:%[0-9]+]]:rgpr = t2ADDri [[COPY3]], 4, 14 /* CC::al */, $noreg, $noreg
   ; CHECK-NEXT:   [[VLDRS3:%[0-9]+]]:spr = VLDRS [[COPY3]], 1, 14 /* CC::al */, $noreg :: (load (s32) from %ir.scevgep3)
-  ; CHECK-NEXT:   INLINEASM &nop, 0 /* attdialect */, 196618 /* regdef:SPR */, def %30, 2147483657 /* reguse tiedto:$0 */, [[VLDRS2]]
+  ; CHECK-NEXT:   INLINEASM &nop, 0 /* attdialect */, 196618 /* regdef:SPR */, def %30, 2147483657 /* reguse tiedto:$0 */, [[VLDRS2]](tied-def 3)
   ; CHECK-NEXT:   [[t2SUBri2:%[0-9]+]]:rgpr = t2SUBri [[COPY]], 1, 14 /* CC::al */, $noreg, def $cpsr
   ; CHECK-NEXT:   [[COPY5:%[0-9]+]]:gprnopc = COPY [[t2SUBri2]]
   ; CHECK-NEXT:   [[COPY6:%[0-9]+]]:gprnopc = COPY [[t2ADDri1]]
   ; CHECK-NEXT:   [[COPY7:%[0-9]+]]:gprnopc = COPY [[t2ADDri]]
   ; CHECK-NEXT:   t2Bcc %bb.9, 0 /* CC::eq */, $cpsr
   ; CHECK-NEXT:   t2B %bb.6, 14 /* CC::al */, $noreg
   ; CHECK-NEXT: {{  $}}
   ; CHECK-NEXT: bb.6.for.body:
   ; CHECK-NEXT:   successors: %bb.7(0x80000000), %bb.8(0x00000000)
   ; CHECK-NEXT: {{  $}}
   ; CHECK-NEXT:   [[VMULS:%[0-9]+]]:spr = nnan ninf nsz arcp contract afn reassoc VMULS [[VLDRS3]], %30, 14 /* CC::al */, $noreg
   ; CHECK-NEXT:   [[t2ADDri2:%[0-9]+]]:rgpr = t2ADDri [[COPY7]], 4, 14 /* CC::al */, $noreg, $noreg
   ; CHECK-NEXT:   [[VLDRS4:%[0-9]+]]:spr = VLDRS [[COPY7]], 1, 14 /* CC::al */, $noreg :: (load unknown-size from %ir.scevgep7, align 4)
   ; CHECK-NEXT:   [[t2ADDri3:%[0-9]+]]:rgpr = t2ADDri [[COPY6]], 4, 14 /* CC::al */, $noreg, $noreg
   ; CHECK-NEXT:   [[VLDRS5:%[0-9]+]]:spr = VLDRS [[COPY6]], 1, 14 /* CC::al */, $noreg :: (load unknown-size from %ir.scevgep3, align 4)
-  ; CHECK-NEXT:   INLINEASM &nop, 0 /* attdialect */, 196618 /* regdef:SPR */, def %40, 2147483657 /* reguse tiedto:$0 */, [[VLDRS4]]
+  ; CHECK-NEXT:   INLINEASM &nop, 0 /* attdialect */, 196618 /* regdef:SPR */, def %40, 2147483657 /* reguse tiedto:$0 */, [[VLDRS4]](tied-def 3)
   ; CHECK-NEXT:   [[t2SUBri3:%[0-9]+]]:rgpr = t2SUBri [[COPY5]], 1, 14 /* CC::al */, $noreg, def $cpsr
   ; CHECK-NEXT:   [[COPY8:%[0-9]+]]:gpr = COPY [[t2SUBri3]]
   ; CHECK-NEXT:   [[COPY9:%[0-9]+]]:gpr = COPY [[t2ADDri3]]
   ; CHECK-NEXT:   [[COPY10:%[0-9]+]]:gpr = COPY [[t2ADDri2]]
   ; CHECK-NEXT:   t2Bcc %bb.8, 0 /* CC::eq */, $cpsr
   ; CHECK-NEXT:   t2B %bb.7, 14 /* CC::al */, $noreg
   ; CHECK-NEXT: {{  $}}
   ; CHECK-NEXT: bb.7.for.body:

llvm/test/CodeGen/Thumb2/pipeliner-preserve-ties.mir

body:             |
   ; CHECK-NEXT:   [[t2LDRSHi12_:%[0-9]+]]:rgpr = t2LDRSHi12 [[t2LDRSH_PRE1]], 2, 14 /* CC::al */, $noreg :: (load (s16) from %ir.uglygep2, !tbaa !5)
   ; CHECK-NEXT:   [[t2SDIV:%[0-9]+]]:rgpr = t2SDIV [[t2LDRSH_PRE]], [[t2LDRSHi12_]], 14 /* CC::al */, $noreg
   ; CHECK-NEXT:   [[t2UXTH:%[0-9]+]]:rgpr = t2UXTH [[t2SDIV]], 0, 14 /* CC::al */, $noreg
   ; CHECK-NEXT:   [[t2MLS:%[0-9]+]]:rgpr = t2MLS [[t2SDIV]], [[t2LDRSHi12_]], [[t2LDRSH_PRE]], 14 /* CC::al */, $noreg
   ; CHECK-NEXT:   [[t2UXTH1:%[0-9]+]]:rgpr = t2UXTH [[t2MLS]], 0, 14 /* CC::al */, $noreg
   ; CHECK-NEXT:   t2STRHi12 [[t2MLS]], [[t2LDRSH_PRE1]], 14, 14 /* CC::al */, $noreg :: (store (s16) into %ir.uglygep4, !tbaa !7)
   ; CHECK-NEXT:   [[t2LDRHi12_:%[0-9]+]]:gprnopc = t2LDRHi12 [[t2LDRSH_PRE1]], 4, 14 /* CC::al */, $noreg :: (load (s16) from %ir.uglygep5, !tbaa !8)
   ; CHECK-NEXT:   t2CMPrr [[t2LDRHi12_]], [[t2UXTH]], 14 /* CC::al */, $noreg, implicit-def $cpsr
-  ; CHECK-NEXT:   [[t2ADDri:%[0-9]+]]:rgpr = t2ADDri [[COPY]], 1, 1 /* CC::ne */, $cpsr, $noreg, implicit [[COPY]]
+  ; CHECK-NEXT:   [[t2ADDri:%[0-9]+]]:rgpr = t2ADDri [[COPY]], 1, 1 /* CC::ne */, $cpsr, $noreg, implicit [[COPY]](tied-def 0)
   ; CHECK-NEXT:   [[t2LDRHi12_1:%[0-9]+]]:gprnopc = t2LDRHi12 [[t2LDRSH_PRE1]], 6, 14 /* CC::al */, $noreg :: (load (s16) from %ir.uglygep6, !tbaa !9)
   ; CHECK-NEXT:   t2CMPrr [[t2LDRHi12_1]], [[t2UXTH1]], 14 /* CC::al */, $noreg, implicit-def $cpsr
-  ; CHECK-NEXT:   [[t2ADDri1:%[0-9]+]]:rgpr = t2ADDri [[t2ADDri]], 1, 1 /* CC::ne */, $cpsr, $noreg, implicit [[t2ADDri]]
+  ; CHECK-NEXT:   [[t2ADDri1:%[0-9]+]]:rgpr = t2ADDri [[t2ADDri]], 1, 1 /* CC::ne */, $cpsr, $noreg, implicit [[t2ADDri]](tied-def 0)
   ; CHECK-NEXT:   [[t2LDRHi12_2:%[0-9]+]]:gprnopc = t2LDRHi12 [[t2LDRSH_PRE1]], 8, 14 /* CC::al */, $noreg :: (load (s16) from %ir.uglygep7, !tbaa !10)
   ; CHECK-NEXT:   t2CMPrr [[t2LDRHi12_2]], [[t2UXTH]], 14 /* CC::al */, $noreg, implicit-def $cpsr
-  ; CHECK-NEXT:   [[t2ADDri2:%[0-9]+]]:rgpr = t2ADDri [[t2ADDri1]], 1, 1 /* CC::ne */, $cpsr, $noreg, implicit [[t2ADDri1]]
+  ; CHECK-NEXT:   [[t2ADDri2:%[0-9]+]]:rgpr = t2ADDri [[t2ADDri1]], 1, 1 /* CC::ne */, $cpsr, $noreg, implicit [[t2ADDri1]](tied-def 0)
   ; CHECK-NEXT:   [[t2LDRHi12_3:%[0-9]+]]:gprnopc = t2LDRHi12 [[t2LDRSH_PRE1]], 10, 14 /* CC::al */, $noreg :: (load (s16) from %ir.uglygep8, !tbaa !11)
   ; CHECK-NEXT:   t2CMPrr [[t2LDRHi12_3]], [[t2UXTH1]], 14 /* CC::al */, $noreg, implicit-def $cpsr
-  ; CHECK-NEXT:   [[t2ADDri3:%[0-9]+]]:rgpr = t2ADDri [[t2ADDri2]], 1, 1 /* CC::ne */, $cpsr, $noreg, implicit [[t2ADDri2]]
+  ; CHECK-NEXT:   [[t2ADDri3:%[0-9]+]]:rgpr = t2ADDri [[t2ADDri2]], 1, 1 /* CC::ne */, $cpsr, $noreg, implicit [[t2ADDri2]](tied-def 0)
   ; CHECK-NEXT:   [[COPY2:%[0-9]+]]:gpr = COPY [[t2ADDri3]]
   ; CHECK-NEXT:   [[t2LDRHi12_4:%[0-9]+]]:gprnopc = t2LDRHi12 [[t2LDRSH_PRE1]], 18, 14 /* CC::al */, $noreg :: (load (s16) from %ir.uglygep9, !tbaa !5)
   ; CHECK-NEXT:   t2CMPri [[t2LDRHi12_4]], 0, 14 /* CC::al */, $noreg, implicit-def $cpsr
   ; CHECK-NEXT:   t2Bcc %bb.5, 0 /* CC::eq */, $cpsr
   ; CHECK-NEXT:   t2B %bb.4, 14 /* CC::al */, $noreg
   ; CHECK-NEXT: {{  $}}
   ; CHECK-NEXT: bb.4.do.body:
   ; CHECK-NEXT:   successors: %bb.5(0x04000000), %bb.4(0x7c000000)
body:             |
   ; CHECK-NEXT:   [[t2LDRHi12_7:%[0-9]+]]:gprnopc = t2LDRHi12 [[t2LDRSH_PRE3]], 10, 14 /* CC::al */, $noreg :: (load unknown-size from %ir.uglygep8, align 2, !tbaa !11)
   ; CHECK-NEXT:   [[t2SDIV1:%[0-9]+]]:rgpr = t2SDIV [[t2LDRSH_PRE2]], [[t2LDRSHi12_1]], 14 /* CC::al */, $noreg
   ; CHECK-NEXT:   [[t2LDRHi12_8:%[0-9]+]]:gprnopc = t2LDRHi12 [[t2LDRSH_PRE3]], 4, 14 /* CC::al */, $noreg :: (load unknown-size from %ir.uglygep5, align 2, !tbaa !8)
   ; CHECK-NEXT:   [[t2MLS1:%[0-9]+]]:rgpr = t2MLS [[t2SDIV1]], [[t2LDRSHi12_1]], [[t2LDRSH_PRE2]], 14 /* CC::al */, $noreg
   ; CHECK-NEXT:   [[t2UXTH2:%[0-9]+]]:rgpr = t2UXTH [[t2SDIV1]], 0, 14 /* CC::al */, $noreg
   ; CHECK-NEXT:   [[t2LDRHi12_9:%[0-9]+]]:gprnopc = t2LDRHi12 [[t2LDRSH_PRE3]], 6, 14 /* CC::al */, $noreg :: (load unknown-size from %ir.uglygep6, align 2, !tbaa !9)
   ; CHECK-NEXT:   t2CMPrr [[t2LDRHi12_8]], [[t2UXTH2]], 14 /* CC::al */, $noreg, implicit-def $cpsr
   ; CHECK-NEXT:   [[t2UXTH3:%[0-9]+]]:rgpr = t2UXTH [[t2MLS1]], 0, 14 /* CC::al */, $noreg
-  ; CHECK-NEXT:   [[t2ADDri4:%[0-9]+]]:rgpr = t2ADDri [[PHI1]], 1, 1 /* CC::ne */, $cpsr, $noreg, implicit [[PHI1]]
+  ; CHECK-NEXT:   [[t2ADDri4:%[0-9]+]]:rgpr = t2ADDri [[PHI1]], 1, 1 /* CC::ne */, $cpsr, $noreg, implicit [[PHI1]](tied-def 0)
   ; CHECK-NEXT:   t2CMPrr [[t2LDRHi12_9]], [[t2UXTH3]], 14 /* CC::al */, $noreg, implicit-def $cpsr
-  ; CHECK-NEXT:   [[t2ADDri5:%[0-9]+]]:rgpr = t2ADDri [[t2ADDri4]], 1, 1 /* CC::ne */, $cpsr, $noreg, implicit [[t2ADDri4]]
+  ; CHECK-NEXT:   [[t2ADDri5:%[0-9]+]]:rgpr = t2ADDri [[t2ADDri4]], 1, 1 /* CC::ne */, $cpsr, $noreg, implicit [[t2ADDri4]](tied-def 0)
   ; CHECK-NEXT:   t2CMPrr [[t2LDRHi12_6]], [[t2UXTH2]], 14 /* CC::al */, $noreg, implicit-def $cpsr
-  ; CHECK-NEXT:   [[t2ADDri6:%[0-9]+]]:rgpr = t2ADDri [[t2ADDri5]], 1, 1 /* CC::ne */, $cpsr, $noreg, implicit [[t2ADDri5]]
+  ; CHECK-NEXT:   [[t2ADDri6:%[0-9]+]]:rgpr = t2ADDri [[t2ADDri5]], 1, 1 /* CC::ne */, $cpsr, $noreg, implicit [[t2ADDri5]](tied-def 0)
   ; CHECK-NEXT:   t2CMPrr [[t2LDRHi12_7]], [[t2UXTH3]], 14 /* CC::al */, $noreg, implicit-def $cpsr
-  ; CHECK-NEXT:   [[t2ADDri7:%[0-9]+]]:rgpr = t2ADDri [[t2ADDri6]], 1, 1 /* CC::ne */, $cpsr, $noreg, implicit [[t2ADDri6]]
+  ; CHECK-NEXT:   [[t2ADDri7:%[0-9]+]]:rgpr = t2ADDri [[t2ADDri6]], 1, 1 /* CC::ne */, $cpsr, $noreg, implicit [[t2ADDri6]](tied-def 0)
   ; CHECK-NEXT:   t2CMPri [[t2LDRHi12_5]], 0, 14 /* CC::al */, $noreg, implicit-def $cpsr
   ; CHECK-NEXT:   [[COPY3:%[0-9]+]]:gpr = COPY [[t2ADDri7]]
   ; CHECK-NEXT:   t2STRHi12 [[t2MLS1]], [[t2LDRSH_PRE3]], 14, 14 /* CC::al */, $noreg :: (store unknown-size into %ir.uglygep4, align 2, !tbaa !7)
   ; CHECK-NEXT:   t2Bcc %bb.4, 1 /* CC::ne */, $cpsr
   ; CHECK-NEXT:   t2B %bb.5, 14 /* CC::al */, $noreg
   ; CHECK-NEXT: {{  $}}
   ; CHECK-NEXT: bb.5:
   ; CHECK-NEXT:   successors: %bb.2(0x80000000)