[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

846

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,
 
   // TII::foldMemoryOperand will do what we need here for statepoint
   // (fold load into use and remove corresponding def). We will replace
   // uses of removed def with loads (spillAroundUses).
   // For that to work we need to untie def and use to pass it through
   // foldMemoryOperand and signal foldPatchpoint that it is allowed to
   // fold them.
   bool UntieRegs = MI->getOpcode() == TargetOpcode::STATEPOINT;
+  bool UntieRegsHappened = false;
+  auto UntieRegsCheck = [UntieRegsHappened]() {
+    assert(!UntieRegsHappened && "Untie Regs broken");
+    return false;
+  };
 
   // Spill subregs if the target allows it.
   // We always want to spill subregs for stackmap/patchpoint pseudos.
   bool SpillSubRegs = TII.isSubregFoldable() ||
                       MI->getOpcode() == TargetOpcode::STATEPOINT ||
                       MI->getOpcode() == TargetOpcode::PATCHPOINT ||
                       MI->getOpcode() == TargetOpcode::STACKMAP;
 
   // TargetInstrInfo::foldMemoryOperand only expects explicit, non-tied
   // operands.
   SmallVector<unsigned, 8> FoldOps;
   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())
+    if (UntieRegs && MO.isTied()) {
+      UntieRegsHappened = true;
       MI->untieRegOperand(Idx);
+    }
 
     if (!SpillSubRegs && MO.getSubReg())
-      return false;
+      return UntieRegsCheck();
     // We cannot fold a load instruction into a def.
     if (LoadMI && MO.isDef())
-      return false;
+      return UntieRegsCheck();
     // Tied use operands should not be passed to foldMemoryOperand.
     if (!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;
+    return UntieRegsCheck();
 
   MachineInstrSpan MIS(MI, MI->getParent());
 
   MachineInstr *FoldMI =
       LoadMI ? TII.foldMemoryOperand(*MI, FoldOps, *LoadMI, &LIS)
              : TII.foldMemoryOperand(*MI, FoldOps, StackSlot, &LIS, &VRM);
   if (!FoldMI)
-    return false;
+    return UntieRegsCheck();
 
   // 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;