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

API change: embed brand_string in GetX86Info() #198

Closed
wants to merge 1 commit into from

Conversation

gchatelet
Copy link
Collaborator

This is an API change.
It reduces the chance that user passes an invalid buffer to FillBrandString

return false;
static bool HasSecondFMA(const X86Info* info) {
switch (info->model) {
case 0x55: { // Skylake server
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@jeffhammond would it make sense to use the result of GetX86Microarchitecture here instead of hardcoded models?
Granted that we don't yet have an entry for SKX alone (it is currently embedded in SKL)

For instance, I think this code would return true for an AMD processor HasSecondFMA is part of ParseCpuId which is called for both Intel and AMD cpus.

Copy link
Contributor

Choose a reason for hiding this comment

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

I moved to NVIDIA in March and work on ARM CPU stuff now, so I don't have a strong opinion about this topic anymore. @fenrus75 or somebody in his team might be able to help now.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Congrats on your new position !
Waiting for @fenrus75 input then.

@gchatelet gchatelet added the API Change This PR introduced a change in the API that must be documented on next release label Oct 29, 2021
@toor1245
Copy link
Contributor

@gchatelet, do we need to close this PR? Since already merged into master #214

@gchatelet gchatelet closed this Feb 1, 2022
@gchatelet gchatelet added this to the v0.7.0 milestone Mar 8, 2022
@gchatelet gchatelet deleted the remove_fill_brand_string branch March 15, 2022 15:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Change This PR introduced a change in the API that must be documented on next release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants