Skip to content

[AMDGPU] Allow rematerialization of instructions with virtual register uses #124327

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Feb 6, 2025
Merged

[AMDGPU] Allow rematerialization of instructions with virtual register uses #124327

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
91c69c5
[AMDGPU] Allow rematerialization of instructions with virtual registe…
jrbyrnes Jan 27, 2025
7e501d5
Do not allow remat for instructions with dependencies
jrbyrnes Jan 31, 2025
10d53b0
Fix asserts
jrbyrnes Feb 4, 2025
389932b
Add test for omitted subrange
jrbyrnes Feb 5, 2025
ebfde36
Fix expensive check
jrbyrnes Feb 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 8 additions & 8 deletions llvm/lib/Target/AMDGPU/GCNSchedStrategy.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1627,10 +1627,14 @@ bool PreRARematStage::allUsesAvailableAt(const MachineInstr *InstToRemat,
if (!MO.isReg() || !MO.getReg() || !MO.readsReg())
continue;

// Do not attempt to reason about PhysRegs
if (!MO.getReg().isVirtual()) {
assert(DAG.MRI.isConstantPhysReg(MO.getReg()) ||
DAG.TII->isIgnorableUse(MO));
// Do not attempt to reason about PhysRegs
// TODO: better analysis of PhysReg livness
if (!DAG.MRI.isConstantPhysReg(MO.getReg()) &&
!DAG.TII->isIgnorableUse(MO))
return false;

// Constant PhysRegs and IgnorableUses are okay
continue;
}

Expand Down Expand Up @@ -1664,7 +1668,6 @@ bool PreRARematStage::allUsesAvailableAt(const MachineInstr *InstToRemat,
if (LM.none())
break;
}
assert(LM.none());
Copy link
Contributor Author

@jrbyrnes jrbyrnes Feb 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Testing had issues with this assert -- would like to understand why -- looking into it.

Copy link
Contributor Author

@jrbyrnes jrbyrnes Feb 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may have non empty LM if our register does not have a subrange for every subregister. I've attached a reproducer of this condition (admittedly, it looks a bit funky but it represents a case in hipBlender).

If a superregister has undef subregisters, LIS won't produce a subrange for those subregisters. Thus, when we walk over the subranges for a full register use, the sum of the subrange masks will not be the full register mask.

}
}
return true;
Expand Down Expand Up @@ -1856,10 +1859,7 @@ bool PreRARematStage::sinkTriviallyRematInsts(const GCNSubtarget &ST,
MachineBasicBlock::iterator InsertPos =
MachineBasicBlock::iterator(It.second);
Register Reg = Def->getOperand(0).getReg();
// Rematerialize MI to its use block. Since we are only rematerializing
// instructions that do not have any virtual reg uses, we do not need to
// call LiveRangeEdit::allUsesAvailableAt() and
// LiveRangeEdit::canRematerializeAt().
// Rematerialize MI to its use block.
TII->reMaterialize(*InsertPos->getParent(), InsertPos, Reg,
Def->getOperand(0).getSubReg(), *Def, *DAG.TRI);
MachineInstr *NewMI = &*std::prev(InsertPos);
Expand Down