Skip to content

[RISCV] Correct the limit of RegPresureSet GPRAll #118473

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

Closed
wants to merge 3 commits into from
Closed

[RISCV] Correct the limit of RegPresureSet GPRAll #118473

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
17117c1
[RISCV] Correct the limit of RegPresureSet `GPRAll`
wangpc-pp Dec 3, 2024
e96f7f7
Test commit: add a parameter to keep reserved
wangpc-pp Dec 3, 2024
6b0461f
Revert "Test commit: add a parameter to keep reserved"
wangpc-pp Dec 3, 2024
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
14 changes: 14 additions & 0 deletions llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -934,3 +934,17 @@ bool RISCVRegisterInfo::getRegAllocationHints(

return BaseImplRetVal;
}

unsigned RISCVRegisterInfo::getRegPressureSetLimit(const MachineFunction &MF,
unsigned Idx) const {
if (Idx == RISCV::RegisterPressureSets::GPRAll) {
unsigned Reserved = 0;
BitVector ReservedRegs = getReservedRegs(MF);
Copy link
Contributor

Choose a reason for hiding this comment

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

This should query the reserved registers from MachineRegisterInfo instead of doing a fresh computation of the set

for (MCPhysReg Reg = RISCV::X0_H; Reg <= RISCV::X31_H; Reg++)
if (ReservedRegs.test(Reg))
Reserved++;

return 32 - Reserved;
}
return RISCVGenRegisterInfo::getRegPressureSetLimit(MF, Idx);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd expect the default to handle reserved registers correctly

Copy link
Member

Choose a reason for hiding this comment

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

The generated version in my build tree looks like the following:

// Get the register unit pressure limit for this dimension.
// This limit must be adjusted dynamically for reserved registers.
unsigned RISCVGenRegisterInfo::
getRegPressureSetLimit(const MachineFunction &MF, unsigned Idx) const {
  static const uint8_t PressureLimitTable[] = {
    2,  	// 0: GPRC_and_SR07
    2,  	// 1: GPRX0
    2,  	// 2: SP
    2,  	// 3: GPRX7
    3,  	// 4: GPRX1
    8,  	// 5: FPR16C
    8,  	// 6: GPRF16C
    8,  	// 7: SR07
    8,  	// 8: VMV0
    14,  	// 9: GPRF16C_with_SR07
    16,  	// 10: GPRTC
    24,  	// 11: VRM8NoV0
    32,  	// 12: FPR16
    32,  	// 13: VM
    33,  	// 14: GPRAll
  };
  return PressureLimitTable[Idx];
}

So it is not correctly handling dynamically reserved registers.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, so fix that

Copy link
Contributor

Choose a reason for hiding this comment

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

RegisterClassInfo has computePSetLimit, is something not using that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

RegisterClassInfo has computePSetLimit, is something not using that?

Yes, please see my comment above. TargetRegisterInfo::getRegPressureSetLimit is used directly in MachineLICM, MachineSink, MachinePipeliner, etc.

Copy link
Contributor

@arsenm arsenm Dec 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This API is a mess. I would expect the TRI to be an implementation detail never directly used. This effectively reimplements the same thing in 2 places

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I created #118787 to fix this.

}
2 changes: 2 additions & 0 deletions llvm/lib/Target/RISCV/RISCVRegisterInfo.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,8 @@ struct RISCVRegisterInfo : public RISCVGenRegisterInfo {
static bool isRVVRegClass(const TargetRegisterClass *RC) {
return RISCVRI::isVRegClass(RC->TSFlags);
}
unsigned getRegPressureSetLimit(const MachineFunction &MF,
unsigned Idx) const override;
};
} // namespace llvm

Expand Down
Loading
Loading