-
Notifications
You must be signed in to change notification settings - Fork 46
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
[WIP] Micro architecture parsing #177
base: master
Are you sure you want to change the base?
[WIP] Micro architecture parsing #177
Conversation
This is meant to enable better API stability if we add partial support for other CPU vendors, such as VIA or Cyrix.
This module supports parsing the CPU family, model and stepping to identify the micro-architecture. This requires listing all the known micro-architecture in the library. (Recompilation will be needed to add support for new uarch) Because Intel micro-architecture are rather messy, especially around Skylake, microarchitecture are mapped onto a structure referencing - the vendor - the CPU core design (or core design for hybrid CPUs such as Alder Lake) - the micro-architecture codename of the SoC. For instance AlderLake is {Intel, Heterogeneous{P: GoldenCove, E: Gracemont}, AlderLake} All the different Skylake recycling are {Intel, Homogenous(Skylake), _) As teh set of combination recognized is fixed at compile time, they are encoded as constants and the function returns static references to the correct structure. All the known combination are included in an array (MICRO_ARCHITECTURE_LIST) The list of micro-architectures in the Enums is rather exhaustive but not definitive, but only a token number of architectures are currently parsed.
Before I implement the whole parsing, I wouldn't mind a review of the design. I am also considering changing Heterogeneous to Hybrid, and Homogenous to something simpler to spell, if you have any idea. @gz for a first review. |
Note, because adding more vendor in the future seems possible, I've marked the Vendor enum as non_exhaustive, and all the micro-architecture ones will also be non_exhaustive. This is a breaking change for users, a major version will be needed, but then adding new vendors or micro-architectures will not require breaking changes as they would otherwise have. |
- Export the uarch module - Export the Vendor enum
- Split NetBurst between 32-bit and 64-bit architectures (the latter identified as Prescott) - Fix typo on Willamette
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.
hey, nice work! I added some comments
where | ||
F: Fn(u32, u32) -> CpuIdResult + Clone, | ||
where | ||
F: Fn(u32, u32) -> CpuIdResult + Clone, |
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.
is this from cargo fmt?
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.
I will check, but I would bet on my IDE running rustfmt. (JetBrain’s Rust Rover)
#[derive(Debug, Eq, PartialEq, Clone, Copy)] | ||
enum Vendor { | ||
pub enum Vendor { |
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.
we should try to have docs for all public types
use crate::Vendor::{Intel, Amd}; | ||
|
||
#[non_exhaustive] | ||
pub enum CoreArch { |
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.
will need docs
pub enum CoreArch { | ||
// Not including Intel micro-architecture without CPUID suport, for now. | ||
// Intel Micro-architectures (with CPUID support) | ||
i486, |
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.
there is some naming warning for this
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.
variant i486
should have an upper camel case name
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.
Maybe we can make sure we match with information from here https://github.com/intel/perfmon?
use crate::Vendor::{Intel, Amd}; | ||
|
||
#[non_exhaustive] | ||
pub enum CoreArch { |
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.
This seems to be sometimes using the "prototype" names for the platform and sometimes the "official" product names, is there some guidelines you followed for picking them?
#[non_exhaustive] | ||
pub enum Core { | ||
Homogenous(CoreArch), | ||
Heterogeneous { P: CoreArch, E: CoreArch }, |
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.
this could maybe just be Vec<CoreArch>
or &static [CoreArch]
? seems tricky to be sure there are only two cores especially in the future
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.
That was one of the options, I felt I was perhaps a bit more painful to handle, but I will make that change. I think ARM land has some CPUs with three types of core too.
} | ||
|
||
#[non_exhaustive] | ||
pub enum UArch { |
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.
maybe call this MicroArchitecture?
Zen2, | ||
Zen3, | ||
Zen4, | ||
|
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.
drop empty line?
|
||
} | ||
|
||
pub struct MicroArchitecture { |
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.
This seems to be the same name as UArch?
// =================== | ||
// Micro-Architectures | ||
// =================== | ||
const INTEL_486: MicroArchitecture = MicroArchitecture { |
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.
maybe all this could be parsed at build time from some csv file and generated with the phf
library or similar?
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.
(not strictly necessary of course, just wondering if it maybe makes things easier)
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.
I definitely remarked that my design required a lot of boilerplate that could be autogenerated.
Is this breaking change (due to non_exhaustive attribute on vendor) necessary? If not, it might be better to have at least one version without breaking changes (minor version update). Then, introduce this breaking change with major version update? |
I think currently the vendor enum is not public, which should make it non breaking (but I am not confident about it). If it is, however, I agree with splitting this into 2 steps. |
Implement micro-architecture parsing, as proposed in #176
This design attempts to capture the subtleties of Skylake and Alder Lake (Hybrid SoCs, and many different SoCs variation using the same core design).