[RegAllocGreedy] Rematerialization crashes while updating operands in bundled MIs

[dshtilman]

Summary

Hi Quentin,

This patch fixes rematerialization in InlineSpiller so it can handle bundles of MIs properly.

Context:
Before InlineSpiller rematerializes a vreg, it iterates over operands of each MI in a bundle, collecting all (MI, OpNo) pairs that reference that vreg.

Then if it does rematerialize, it goes through the pair list and replaces the operands with the new (rematerialized) vreg. The problem is, it tries to replace all of these operands in the main MI ! This works fine for single MIs. However, if we are processing a bundle of MIs and the list contains multiple pairs - the rematerialization will either crash trying to access a non-existing operand of the main MI, or silently corrupt one of the existing ones. It will also ignore other MIs in the bundle.

The obvious fix is to use the MI pointers saved in collected (MI, OpNo) pairs. This must have been the original intent of the pair list but somehow these pointers got lost.

Please review.

Thanks,
Dmitri

[dshtilman]

Hi Quentin,

This patch fixes rematerialization in InlineSpiller so it can handle bundles of MIs properly.

Context:
Before InlineSpiller rematerializes a vreg, it iterates over operands of each MI in a bundle, collecting all (MI, OpNo) pairs that reference that vreg.

Then if it does rematerialize, it goes through the pair list and replaces the operands with the new (rematerialized) vreg. The problem is, it tries to replace all of these operands in the main MI ! This works fine for single MIs. However, if we are processing a bundle of MIs and the list contains multiple pairs - the rematerialization will either crash trying to access a non-existing operand of the main MI, or silently corrupt one of the existing ones. It will also ignore other MIs in the bundle.

The obvious fix is to use the MI pointers saved in collected (MI, OpNo) pairs. This must have been the original intent of the pair list but somehow these pointers got lost.

Please review.

Thanks,
Dmitri

[qcolombet]

Hi Dmitri,

The bug is scary!

This LGTM to a few nitpicks while you are here.
Also I assume this is not triggered by any in-tree target. If it is, please add a test case.

Thanks,
-Quentin

lib/CodeGen/InlineSpiller.cpp

924

MI is not reused anywhere so just fold the expression here:
MachineOperand &MO = Ops[i].first->getOperand(Ops[i].second);

That way we won’t have the shadow variable.

1102–1103

While you are here, add an assert in that loop:
MI == Ops[i].first

[dshtilman]

Scary indeed!

I addressed your comments. This hasn't been triggered by an in-tree target,
so at this point I don't have any tests that I could add.

The loop in InlineSpiller::foldMemoryOperand() is actually guarded by

// Don't attempt folding in bundles.
MachineInstr *MI = Ops.front().first;
if (Ops.back().first != MI || MI->isBundled())
  return false;

so we're unlikely to run into similar issues in this loop.
But I agree it's good to have an extra assert here just to be sure.

Thanks,
Dmitri

[qcolombet]

Thanks Dmitri!

Do you want me to commit on your behalf?

Cheers,
-Quentin

[dshtilman]

Yes, please do.

Thanks,
Dmitri

[Eugene.Zelenko]

Committed in rL237964.

lib/CodeGen/InlineSpiller.cpp

bool InlineSpiller::reMaterializeFor(LiveInterval &VirtReg,
   SlotIndex DefIdx = Edit->rematerializeAt(*MI->getParent(), MI, NewVReg, RM,
                                            TRI);
   (void)DefIdx;
   DEBUG(dbgs() << "\tremat:  " << DefIdx << '\t'
                << *LIS.getInstructionFromIndex(DefIdx));
 
   // Replace operands
   for (unsigned i = 0, e = Ops.size(); i != e; ++i) {
-    MachineOperand &MO = MI->getOperand(Ops[i].second);
+    MachineOperand &MO = Ops[i].first->getOperand(Ops[i].second);
     if (MO.isReg() && MO.isUse() && MO.getReg() == VirtReg.reg) {
       MO.setReg(NewVReg);
       MO.setIsKill();
     }
   }
   DEBUG(dbgs() << "\t        " << UseIdx << '\t' << *MI << '\n');
 
   ++NumRemats;
foldMemoryOperand(ArrayRef<std::pair<MachineInstr*, unsigned> > Ops,
   bool SpillSubRegs = (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 (unsigned i = 0, e = Ops.size(); i != e; ++i) {
     unsigned Idx = Ops[i].second;
+    assert(MI == Ops[i].first && "Instruction conflict during operand folding");
     MachineOperand &MO = MI->getOperand(Idx);
     if (MO.isImplicit()) {
       ImpReg = MO.getReg();
       continue;
     }
     // FIXME: Teach targets to deal with subregs.
     if (!SpillSubRegs && MO.getSubReg())
       return false;