Skip to content

[AMDGPU] Delete redundant s_or_b32 #165261

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 15 commits into from
Nov 7, 2025
Merged

[AMDGPU] Delete redundant s_or_b32 #165261

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
d9321dd
Delete redundant s_or_b32
LU-JOHN Oct 22, 2025
8f79d82
Fix mnemonic
LU-JOHN Oct 27, 2025
a276310
Fix arguments
LU-JOHN Nov 1, 2025
f261b26
Fix comment
LU-JOHN Nov 1, 2025
b4f3449
Don't copy MachineOperand
LU-JOHN Nov 1, 2025
b3db724
Make comment clear
LU-JOHN Nov 1, 2025
57d9edd
Ensure hi/lo halves are or-ed. Add mir tests.
LU-JOHN Nov 3, 2025
919962b
Add undef testcase
LU-JOHN Nov 5, 2025
7dbdbe4
Fix handling of undef
LU-JOHN Nov 5, 2025
92f73c6
Use getVRegDef
LU-JOHN Nov 5, 2025
3f8d209
Use correct instruction for scan start point
LU-JOHN Nov 5, 2025
d815e12
Ensure scan points have same parent
LU-JOHN Nov 5, 2025
8889e21
Add negative test for intervening scc def
LU-JOHN Nov 5, 2025
f2e16ad
Fix grammar
LU-JOHN Nov 5, 2025
9cea63d
remove braces
LU-JOHN Nov 5, 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
28 changes: 27 additions & 1 deletion llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10160,7 +10160,7 @@ static bool followSubRegDef(MachineInstr &MI,
}

MachineInstr *llvm::getVRegSubRegDef(const TargetInstrInfo::RegSubRegPair &P,
MachineRegisterInfo &MRI) {
const MachineRegisterInfo &MRI) {
assert(MRI.isSSA());
if (!P.Reg.isVirtual())
return nullptr;
Expand Down Expand Up @@ -10689,6 +10689,32 @@ bool SIInstrInfo::optimizeCompareInstr(MachineInstr &CmpInstr, Register SrcReg,
if (!optimizeSCC(Def, &CmpInstr, RI))
return false;

// If s_or_b32 result, sY, is unused (i.e. it is effectively a 64-bit
// s_cmp_lg of a register pair) and the inputs are the hi and lo-halves of a
// 64-bit foldableSelect then delete s_or_b32 in the sequence:
// sX = s_cselect_b64 (non-zero imm), 0
// sLo = copy sX.sub0
// sHi = copy sX.sub1
// sY = s_or_b32 sLo, sHi
Copy link
Contributor

@shiltian shiltian Nov 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This pattern is very specific. If sY is dead, won't all its operands be dead automatically (if there is no other uses)? Why do we need to have a special handling here?

Copy link
Contributor Author

@LU-JOHN LU-JOHN Nov 5, 2025

Choose a reason for hiding this comment

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

s_or_b32 may still be alive because it defines scc. This optimization ensures that the scc def is redundant because it is already available after the s_cselect_b64.

Copy link
Contributor Author

@LU-JOHN LU-JOHN Nov 5, 2025

Choose a reason for hiding this comment

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

It is intended to deal with redundant s_or_b32s that are generated when lowering 64-bit add/sub on R600 targets. R600 does not have a 64-bit s_cmp, so the s_or_b32 is used in the lowering.

Copy link
Contributor Author

@LU-JOHN LU-JOHN Nov 5, 2025

Choose a reason for hiding this comment

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

See llvm/test/CodeGen/AMDGPU/carryout-selection.ll for an example of why this optimization is needed.

if (Def->getOpcode() == AMDGPU::S_OR_B32 &&
MRI->use_nodbg_empty(Def->getOperand(0).getReg())) {
const MachineOperand &OrOpnd1 = Def->getOperand(1);
const MachineOperand &OrOpnd2 = Def->getOperand(2);
if (OrOpnd1.isReg() && OrOpnd2.isReg()) {
MachineInstr *Def1 = MRI->getUniqueVRegDef(OrOpnd1.getReg());
MachineInstr *Def2 = MRI->getUniqueVRegDef(OrOpnd2.getReg());
Copy link
Contributor

@arsenm arsenm Nov 5, 2025

Choose a reason for hiding this comment

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

Need to check if there failed, this will crash on undef operands. Probably should also use getVRegDef, this doesn't run on post-SSA

Copy link
Contributor Author

@LU-JOHN LU-JOHN Nov 5, 2025

Choose a reason for hiding this comment

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

Added test with undef operands which crashed without checks.
Updated to check if Def1 and Def2 are non-null.
Change to getVRegDef.

if (Def1->getOpcode() == AMDGPU::COPY &&
Def2->getOpcode() == AMDGPU::COPY && Def1->getOperand(1).isReg() &&
Def2->getOperand(1).isReg() &&
Def1->getOperand(1).getSubReg() == AMDGPU::sub0 &&
Def2->getOperand(1).getSubReg() == AMDGPU::sub1 &&
Def1->getOperand(1).getReg() == Def2->getOperand(1).getReg() &&
foldableSelect(
*MRI->getUniqueVRegDef(Def1->getOperand(1).getReg()))) {
Copy link
Contributor

@arsenm arsenm Nov 5, 2025

Choose a reason for hiding this comment

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

Ditto

Copy link
Contributor Author

@LU-JOHN LU-JOHN Nov 5, 2025

Choose a reason for hiding this comment

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

Changed.

optimizeSCC(Def1, Def, RI);
}
}
}
return true;
};

Expand Down
2 changes: 1 addition & 1 deletion llvm/lib/Target/AMDGPU/SIInstrInfo.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1687,7 +1687,7 @@ TargetInstrInfo::RegSubRegPair getRegSequenceSubReg(MachineInstr &MI,
/// skipping copy like instructions and subreg-manipulation pseudos.
/// Following another subreg of a reg:subreg isn't supported.
MachineInstr *getVRegSubRegDef(const TargetInstrInfo::RegSubRegPair &P,
MachineRegisterInfo &MRI);
const MachineRegisterInfo &MRI);

/// \brief Return false if EXEC is not changed between the def of \p VReg at \p
/// DefMI and the use at \p UseMI. Should be run on SSA. Currently does not
Expand Down
Loading
Loading