Skip to content

[AMDGPU] Reduce use of continue in SIWholeQuadMode. NFC. #93659

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 1 commit into from
May 29, 2024
Merged

[AMDGPU] Reduce use of continue in SIWholeQuadMode. NFC. #93659

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
92 changes: 36 additions & 56 deletions llvm/lib/Target/AMDGPU/SIWholeQuadMode.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -278,11 +278,10 @@ LLVM_DUMP_METHOD void SIWholeQuadMode::printInfo() {

for (const MachineInstr &MI : *BII.first) {
auto III = Instructions.find(&MI);
if (III == Instructions.end())
continue;

dbgs() << " " << MI << " Needs = " << PrintState(III->second.Needs)
<< ", OutNeeds = " << PrintState(III->second.OutNeeds) << '\n';
if (III != Instructions.end()) {
dbgs() << " " << MI << " Needs = " << PrintState(III->second.Needs)
<< ", OutNeeds = " << PrintState(III->second.OutNeeds) << '\n';
}
}
}
}
Expand Down Expand Up @@ -455,10 +454,8 @@ void SIWholeQuadMode::markOperand(const MachineInstr &MI,
for (MCRegUnit Unit : TRI->regunits(Reg.asMCReg())) {
LiveRange &LR = LIS->getRegUnit(Unit);
const VNInfo *Value = LR.Query(LIS->getInstructionIndex(MI)).valueIn();
if (!Value)
continue;

markDefs(MI, LR, Unit, AMDGPU::NoSubRegister, Flag, Worklist);
if (Value)
markDefs(MI, LR, Unit, AMDGPU::NoSubRegister, Flag, Worklist);
}
}
}
Expand Down Expand Up @@ -499,19 +496,16 @@ char SIWholeQuadMode::scanInstructions(MachineFunction &MF,

if (TII->isWQM(Opcode)) {
// If LOD is not supported WQM is not needed.
if (!ST->hasExtendedImageInsts())
continue;
// Only generate implicit WQM if implicit derivatives are required.
// This avoids inserting unintended WQM if a shader type without
// implicit derivatives uses an image sampling instruction.
if (!HasImplicitDerivatives)
continue;
// Sampling instructions don't need to produce results for all pixels
// in a quad, they just require all inputs of a quad to have been
// computed for derivatives.
markInstructionUses(MI, StateWQM, Worklist);
GlobalFlags |= StateWQM;
continue;
if (ST->hasExtendedImageInsts() && HasImplicitDerivatives) {
// Sampling instructions don't need to produce results for all pixels
// in a quad, they just require all inputs of a quad to have been
// computed for derivatives.
markInstructionUses(MI, StateWQM, Worklist);
GlobalFlags |= StateWQM;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Alternatively, remove all the else after continue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That seems like it would be tricky and confusing because not all of the cases continue. The ones that set Flags do not continue. Hence I removed all the continues and added if (Flags) around the small bit of code at the bottom of the loop.

} else if (Opcode == AMDGPU::WQM) {
// The WQM intrinsic requires its output to have all the helper lanes
// correct, so we need it to be in WQM.
Expand All @@ -520,15 +514,13 @@ char SIWholeQuadMode::scanInstructions(MachineFunction &MF,
} else if (Opcode == AMDGPU::SOFT_WQM) {
LowerToCopyInstrs.push_back(&MI);
SoftWQMInstrs.push_back(&MI);
continue;
} else if (Opcode == AMDGPU::STRICT_WWM) {
// The STRICT_WWM intrinsic doesn't make the same guarantee, and plus
// it needs to be executed in WQM or Exact so that its copy doesn't
// clobber inactive lanes.
markInstructionUses(MI, StateStrictWWM, Worklist);
GlobalFlags |= StateStrictWWM;
LowerToMovInstrs.push_back(&MI);
continue;
} else if (Opcode == AMDGPU::STRICT_WQM ||
TII->isDualSourceBlendEXP(MI)) {
// STRICT_WQM is similar to STRICTWWM, but instead of enabling all
Expand All @@ -551,7 +543,6 @@ char SIWholeQuadMode::scanInstructions(MachineFunction &MF,
GlobalFlags |= StateExact;
III.Disabled = StateWQM | StateStrict;
}
continue;
} else if (Opcode == AMDGPU::LDS_PARAM_LOAD ||
Opcode == AMDGPU::DS_PARAM_LOAD ||
Opcode == AMDGPU::LDS_DIRECT_LOAD ||
Expand All @@ -561,7 +552,6 @@ char SIWholeQuadMode::scanInstructions(MachineFunction &MF,
InstrInfo &II = Instructions[&MI];
II.Needs |= StateStrictWQM;
GlobalFlags |= StateStrictWQM;
continue;
} else if (Opcode == AMDGPU::V_SET_INACTIVE_B32 ||
Opcode == AMDGPU::V_SET_INACTIVE_B64) {
III.Disabled = StateStrict;
Expand All @@ -574,7 +564,6 @@ char SIWholeQuadMode::scanInstructions(MachineFunction &MF,
}
}
SetInactiveInstrs.push_back(&MI);
continue;
} else if (TII->isDisableWQM(MI)) {
BBI.Needs |= StateExact;
if (!(BBI.InNeeds & StateExact)) {
Expand All @@ -583,40 +572,33 @@ char SIWholeQuadMode::scanInstructions(MachineFunction &MF,
}
GlobalFlags |= StateExact;
III.Disabled = StateWQM | StateStrict;
continue;
} else {
if (Opcode == AMDGPU::SI_PS_LIVE || Opcode == AMDGPU::SI_LIVE_MASK) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The main motivation was to move the if-else chain that starts here "up" into the surrounding if-else chain, because I found it confusing.

LiveMaskQueries.push_back(&MI);
} else if (Opcode == AMDGPU::SI_KILL_I1_TERMINATOR ||
Opcode == AMDGPU::SI_KILL_F32_COND_IMM_TERMINATOR ||
Opcode == AMDGPU::SI_DEMOTE_I1) {
KillInstrs.push_back(&MI);
BBI.NeedsLowering = true;
} else if (WQMOutputs) {
// The function is in machine SSA form, which means that physical
// VGPRs correspond to shader inputs and outputs. Inputs are
// only used, outputs are only defined.
// FIXME: is this still valid?
for (const MachineOperand &MO : MI.defs()) {
if (!MO.isReg())
continue;

Register Reg = MO.getReg();

if (!Reg.isVirtual() &&
TRI->hasVectorRegisters(TRI->getPhysRegBaseClass(Reg))) {
Flags = StateWQM;
break;
}
} else if (Opcode == AMDGPU::SI_PS_LIVE ||
Opcode == AMDGPU::SI_LIVE_MASK) {
LiveMaskQueries.push_back(&MI);
} else if (Opcode == AMDGPU::SI_KILL_I1_TERMINATOR ||
Opcode == AMDGPU::SI_KILL_F32_COND_IMM_TERMINATOR ||
Opcode == AMDGPU::SI_DEMOTE_I1) {
KillInstrs.push_back(&MI);
BBI.NeedsLowering = true;
} else if (WQMOutputs) {
// The function is in machine SSA form, which means that physical
// VGPRs correspond to shader inputs and outputs. Inputs are
// only used, outputs are only defined.
// FIXME: is this still valid?
for (const MachineOperand &MO : MI.defs()) {
Register Reg = MO.getReg();
if (Reg.isPhysical() &&
TRI->hasVectorRegisters(TRI->getPhysRegBaseClass(Reg))) {
Flags = StateWQM;
break;
}
}

if (!Flags)
continue;
}

markInstruction(MI, Flags, Worklist);
GlobalFlags |= Flags;
if (Flags) {
markInstruction(MI, Flags, Worklist);
GlobalFlags |= Flags;
}
}
}

Expand Down Expand Up @@ -1568,8 +1550,6 @@ void SIWholeQuadMode::lowerKillInstrs(bool IsWQM) {
case AMDGPU::SI_KILL_F32_COND_IMM_TERMINATOR:
SplitPoint = lowerKillF32(*MBB, *MI);
break;
default:
continue;
}
if (SplitPoint)
splitBlock(MBB, SplitPoint);
Expand Down
Loading