-
Notifications
You must be signed in to change notification settings - Fork 376
adding-new-intel-cpuids #334
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.
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
base: main
Are you sure you want to change the base?
Conversation
Changed the processor names to the microarchtecture names for consistency and accuracy.
src/x86/uarch.c
Outdated
| case 0xBE: // Alder Lake-N | ||
| return cpuinfo_uarch_gracemont; | ||
| case 0x8F: // Sapphire Rapids (Golden Cove) | ||
| return cpuinfo_uarch_golden_cove; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove - all golden cove can share one return
src/x86/uarch.c
Outdated
| case 0xBE: // Alder Lake N / Raptor Lake N (Golden Cove) | ||
| return cpuinfo_uarch_gracemont; | ||
| case 0xAD: // Granite Rapids (Redood Cove) | ||
| return cpuinfo_uarch_redwood_cove; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove. ditto
| case 0xB5: // Arrow Lake U | ||
| case 0xC5: // Arrow Lake P | ||
| case 0xC6: // Arrow Lake S/HX | ||
| return cpuinfo_uarch_lion_cove; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
| return "Lion Cove"; | ||
| case cpuinfo_uarch_cougar_cove: | ||
| return "Cougar Cove"; | ||
| case cpuinfo_uarch_skymont: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmmm... there are no skymont stand alone soc?
I guess code aiming at lunarlake could check for skymont or lioncove
Removed redundant return for Lunar Lake and adjusted case order.
|
Needs another reviewer to approve |
|
@rrwinterton Can you resolve the clang format errors and rebase? I can merge once checks are green. The CMake failures are caused by CMake 4.x dropping support for older versions and should be resolved on the latest main. Thanks! clang-format job: https://github.com/pytorch/cpuinfo/actions/runs/19049667827/job/54608194604?pr=334 |
In your link clang format points to the end of line: Do you know specifically if the issue is CR/LF vs NL? Does cpuinfo provide a script / command to format? |
Yeah, running clang-format -i [path] should be the easiest way. |
formatting changes to compile with clang-formatting.
| case 0x9C: // Jacobsville | ||
| return cpuinfo_uarch_tremont; | ||
| case 0xBE: // Alder Lake-N | ||
| case 0x8F: // Sapphire Rapids (Golden Cove) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit starting at line 171 is little cores... consider moving to line 170, just after Icelake
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just submitted fix for p-core order and spelling error.
| return cpuinfo_uarch_gracemont; | ||
| case 0xAA: // Meteor Lake P/M (Redwood Cove) | ||
| case 0xAC: // Meteor Lake S (Redwood Cove) | ||
| case 0xAD: // Granite Rapids (Redood Cove) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo, should be *Redwood
|
On my laptop with this Lunar Lake CPU, I get the following result. This appears to be incorrect, because Lunar Lake processors contain a mixture of Lion Cover cores (P-cores) and Skymont (E-cores). |
This is because the infrastructure doesn't support threading to do CPUID for hybrid. The way it works right now is it assume non-hybrid cores. I can fix this by adding affinity to the CPUID when called or we could use a table for the microarchitectures but this is less desirable since it isn't guarenteed to be static. |
|
@rrwinterton wrote:
Additionally, the x86-specific code doesn't even use a data structure which allows it to store multiple uarchs, even though ARM + RISC-V code do use that. I've updated that very basic part for x86 in dlenski/cpuinfo@pr-334...x86_multi_uarch
Agreed. A table would have to be updated for literally every single hybrid SKU in order to not be incorrect. The only way to do this for hybrid CPUs is to run CPUID on each of the cores and record the uarch for that core: "As the Intel docs say: CPUID, by design, returns different values depending on the core it is executed on. On hybrid cores more of the CPUID leaves will have data that varies," |
this PR is to add new Intel Processors that are not included in the cpuinfo. This includes the detection of Alder Lake, Meteor Lake, Arrow Lake, Lunar Lake and Panther Lake mobile platforms as well as Sapphire Rapids and Granite Rapid Server/Workstations