[InlineSpiller]Re-tie operands if folding failed

[skatkov]

Summary

InlineSpiller::foldMemoryOperand unties registers before an attempt to fold and
does not restore tied-ness in case of failure.

I do not have a particular test for demo of invalid behavior. This is something of clean-up.
It is better to keep the behavior correct in case some time in future it happens,

[dantrushin]

LGTM

llvm/lib/CodeGen/InlineSpiller.cpp

840–841

We can move this below LoadMI && MO.isDef() check.

[reames]

I would strongly prefer that instead of simply asserting that we retie the registers. Doing so should be fairly simple - we just need to keep track of which register pairs we untied for the possible fold, and use a lambda to retie them.

Despite that, I am also LGTMing this as an interim step.

Context for those reading along, there's some form of register allocator related miscompile happening in a complicated statepoint example. This bit of code came up in an audit for possible issues - thus the desire to rule it out with asserts. Reducing the example down to a small enough test case to analyze has been challenging.

[skatkov]

Ok, I modified the patch to re-tie operands in case of folding failure.

[dantrushin]

LGTM

[reames]

Also LGTM. Thanks.

llvm/lib/CodeGen/InlineSpiller.cpp

foldMemoryOperand(ArrayRef<std::pair<MachineInstr *, unsigned>> Ops,
   for (const auto &OpPair : Ops) {
     unsigned Idx = OpPair.second;
     assert(MI == OpPair.first && "Instruction conflict during operand folding");
     MachineOperand &MO = MI->getOperand(Idx);
     if (MO.isImplicit()) {
       ImpReg = MO.getReg();
       continue;
     }
 
-    if (UntieRegs && MO.isTied())
-      MI->untieRegOperand(Idx);
-
     if (!SpillSubRegs && MO.getSubReg())
       return false;
     // We cannot fold a load instruction into a def.
     if (LoadMI && MO.isDef())
       return false;
     // Tied use operands should not be passed to foldMemoryOperand.
-    if (!MI->isRegTiedToDefOperand(Idx))
+    if (UntieRegs || !MI->isRegTiedToDefOperand(Idx))
       FoldOps.push_back(Idx);
   }
 
   // If we only have implicit uses, we won't be able to fold that.
   // Moreover, TargetInstrInfo::foldMemoryOperand will assert if we try!
   if (FoldOps.empty())
     return false;
 
   MachineInstrSpan MIS(MI, MI->getParent());
 
+  SmallVector<std::pair<unsigned, unsigned> > TiedOps;
+  if (UntieRegs)
+    for (unsigned Idx : FoldOps) {
+      MachineOperand &MO = MI->getOperand(Idx);
+      if (!MO.isTied())
+        continue;
+      unsigned Tied = MI->findTiedOperandIdx(Idx);
+      if (MO.isUse())
+        TiedOps.emplace_back(Tied, Idx);
+      else {
+        assert(MO.isDef() && "Tied to not use and def?");
+        TiedOps.emplace_back(Idx, Tied);
+      }
+      MI->untieRegOperand(Idx);
+    }
+
   MachineInstr *FoldMI =
       LoadMI ? TII.foldMemoryOperand(*MI, FoldOps, *LoadMI, &LIS)
              : TII.foldMemoryOperand(*MI, FoldOps, StackSlot, &LIS, &VRM);
-  if (!FoldMI)
+  if (!FoldMI) {
+    // Re-tie operands.
+    for (auto Tied : TiedOps)
+      MI->tieOperands(Tied.first, Tied.second);
     return false;
+  }
 
   // Remove LIS for any dead defs in the original MI not in FoldMI.
   for (MIBundleOperands MO(*MI); MO.isValid(); ++MO) {
     if (!MO->isReg())
       continue;
     Register Reg = MO->getReg();
     if (!Reg || Register::isVirtualRegister(Reg) || MRI.isReserved(Reg)) {
       continue;