This is an archive of the discontinued LLVM Phabricator instance.

[VirtRegRewriter] Avoid clobbering registers when expanding copy bundles
ClosedPublic

Authored by bogner on Jun 13 2018, 3:00 PM.

Details

Summary

If a copy bundle happens to involve overlapping registers, we can end up with emitting the copies in an order that ends up clobbering some of the subregisters. Since instructions in the copy bundle semantically happen at the same time, this is incorrect and we need to make sure we order the copies such that this doesn't happen.

Diff Detail

Event Timeline

bogner created this revision.Jun 13 2018, 3:00 PM

Take a look at RAGreedy::selectOrSplit for an example on how to report fatal errors in the backend.

bogner updated this revision to Diff 151273.Jun 13 2018, 5:20 PM
bogner edited the summary of this revision. (Show Details)

Take a look at RAGreedy::selectOrSplit for an example on how to report fatal errors in the backend.

Ah, thanks. Updated with a version that errors instead of ignoring the problem.

MatzeB added inline comments.Jun 13 2018, 5:22 PM
lib/CodeGen/VirtRegMap.cpp
428

unsigned SrcReg instead of const auto&?

430–433

Use TargetRegisterInfo::regsOverlap() for this.

484

I'd vote for less auto, the type isn't immediately clear from this line.

bogner updated this revision to Diff 151304.Jun 13 2018, 11:08 PM
bogner marked 3 inline comments as done.
bogner added inline comments.
lib/CodeGen/VirtRegMap.cpp
428

No longer relevant now that we use regsOverlap().

MatzeB accepted this revision.Jun 14 2018, 11:20 AM

LGTM with this round of nitpicks addressed.

The re-scheduling code is quite tricky so I hope I didn't miss something. On the other hand I don't have an idea on how to make it simpler (except the small stuff mentioned).

lib/CodeGen/VirtRegMap.cpp
426

I think regsOverlap also has a Src==Dst shortcut, so this should be unnecessary. So the 2 ifs can shrink to return TRI->regsOverlap()

451

Maybe give this a more specific name like BundledMI?

455–461

I guess this could just be Inst->removeFromBundle() in both cases (to simplify the code).

457

Could you try if this works without ->getIterator() I think it implicitely converts at least in some situations.

This revision is now accepted and ready to land.Jun 14 2018, 11:20 AM
bogner updated this revision to Diff 151385.Jun 14 2018, 11:33 AM
bogner marked 3 inline comments as done.
bogner added inline comments.
lib/CodeGen/VirtRegMap.cpp
426

This check is for if the src and dst are the same instruction, in which case we're okay with them aliasing. For example, in

$q1_q2_q3 = COPY $q0_q1_q2 {
  $q4 = COPY $q3
}

We aren't interested in the fact that $q1_q2_q3 aliases $q0_q1_q2 - that's the instruction's own operand. What we care about is that $q1_q2_q3 aliases $q3.

455–461

removeFromBundle is a bit confusingly named - it's removeFromParent for bundled instructions. We use unbundleFromSucc here so it stays in the block but stops being bundled.

This revision was automatically updated to reflect the committed changes.

Already saw the bot and reverted, thanks. Fix on the way.

Committed with a fix for the uninitialized variable in r334756.

LuoYuanke added inline comments.
llvm/trunk/test/CodeGen/AArch64/overlapping-copy-bundle-cycle.mir
12 ↗(On Diff #151388)

Anyone knows how this case happen in register allocation? Does RA respect the possible register overlap issue of bundle instruction during the register allocation?

Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptAug 12 2023, 1:28 AM
LuoYuanke added inline comments.Aug 12 2023, 5:31 AM
llvm/trunk/test/CodeGen/AArch64/overlapping-copy-bundle-cycle.mir
12 ↗(On Diff #151388)

After investigating the code the copy instruction was generated in register split. When splitting register and generating copy instructions, RA would bundle the copy instructions. Is there any risk if we don't bundle the copy instructions?

SlotIndex SplitEditor::buildSingleSubRegCopy(Register FromReg, Register ToReg,
    MachineBasicBlock &MBB, MachineBasicBlock::iterator InsertBefore,
    unsigned SubIdx, LiveInterval &DestLI, bool Late, SlotIndex Def) {
  const MCInstrDesc &Desc = TII.get(TargetOpcode::COPY);
  bool FirstCopy = !Def.isValid();
  MachineInstr *CopyMI = BuildMI(MBB, InsertBefore, DebugLoc(), Desc)
      .addReg(ToReg, RegState::Define | getUndefRegState(FirstCopy)
              | getInternalReadRegState(!FirstCopy), SubIdx)
      .addReg(FromReg, 0, SubIdx);

  SlotIndexes &Indexes = *LIS.getSlotIndexes();
  if (FirstCopy) {
    Def = Indexes.insertMachineInstrInMaps(*CopyMI, Late).getRegSlot();
  } else {
    CopyMI->bundleWithPred();
  }
  return Def;
}