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.

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

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

GuillaumeDIDIER
Copy link

@GuillaumeDIDIER GuillaumeDIDIER commented May 26, 2024

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).

  • Intel
  • AMD

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.
@GuillaumeDIDIER
Copy link
Author

GuillaumeDIDIER commented May 26, 2024

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.

@GuillaumeDIDIER
Copy link
Author

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
Copy link
Owner

@gz gz left a 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,
Copy link
Owner

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?

Copy link
Author

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 {
Copy link
Owner

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 {
Copy link
Owner

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,
Copy link
Owner

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

Copy link
Owner

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

Copy link
Owner

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 {
Copy link
Owner

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 },
Copy link
Owner

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

Copy link
Author

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 {
Copy link
Owner

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,

Copy link
Owner

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 {
Copy link
Owner

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 {
Copy link
Owner

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?

Copy link
Owner

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)

Copy link
Author

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.

@mert-kurttutan
Copy link
Contributor

mert-kurttutan commented Jun 4, 2024

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.

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?

@GuillaumeDIDIER
Copy link
Author

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.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants