Skip to content
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

Update PerfCounters.cc #3771

Closed
wants to merge 1 commit into from
Closed

Update PerfCounters.cc #3771

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
2 changes: 2 additions & 0 deletions src/PerfCounters.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@ enum CpuMicroarch {
AMDZen2,
AMDZen3,
AMDZen4,
AMDRyzen9,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't need to add this; this CPU is Zen 4.

Copy link
Author

@20urc3 20urc3 Jun 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Try it. Not supported. I think we need this line to add the following { AMDRyzen9, "AMD Ryzen 9", 0x5100d1, 0, 0x6F, 10000, PMU_TICKS_RCB | PMU_SKID_UNBOUNDED },

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why did you put 0x6f there? That's the llsc_cntr_event and it makes no sense to set that for an x86 CPU.

Using Zen4 should work. If it doesn't, something else is quite broken.

Copy link
Author

Choose a reason for hiding this comment

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

I've just followed the pattern I saw before. After compiling this version it works on my end, so maybe the issue is not fix by my did but was already fixed between the latest package release and the git version?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, this change doesn't actually do anything because nothing detects this new architecture type AMDRyzen9..

LastAMD = AMDZen4,
FirstARM,
ARMNeoverseN1 = FirstARM,
Expand Down Expand Up @@ -203,6 +204,7 @@ static const PmuConfig pmu_configs[] = {
{ AMDZen2, "AMD Zen 2", 0x5100d1, 0, 0, 10000, PMU_TICKS_RCB | PMU_SKID_UNBOUNDED },
{ AMDZen3, "AMD Zen 3", 0x5100d1, 0, 0, 10000, PMU_TICKS_RCB | PMU_SKID_UNBOUNDED },
{ AMDZen4, "AMD Zen 4", 0x5100d1, 0, 0, 10000, PMU_TICKS_RCB | PMU_SKID_UNBOUNDED },
{ AMDRyzen9, "AMD Ryzen 9", 0x5100d1, 0, 0x6F, 10000, PMU_TICKS_RCB | PMU_SKID_UNBOUNDED },
// Performance cores from ARM from cortex-a76 on (including neoverse-n1 and later)
// have the following counters that are reliable enough for us.
// 0x21 == BR_RETIRED - Architecturally retired taken branches
Expand Down
Loading