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

Neoverse V2: remove sm3, sm4 and svesm4 #106

Closed
wants to merge 1 commit into from

Conversation

paolotricerri
Copy link
Contributor

@paolotricerri paolotricerri commented May 22, 2024

fixes #102

@paolotricerri
Copy link
Contributor Author

@dslarm

@giordano
Copy link
Contributor

Can you please edit the title of the PR to be self descriptive? Just referencing an issue number isn't useful.

@alalazo
Copy link
Member

alalazo commented May 23, 2024

Can you please edit the title of the PR to be self descriptive? Just referencing an issue number isn't useful.

Asked the same thing in #105 😅 A brief description of the PR would be welcome too.

@dslarm
Copy link

dslarm commented May 23, 2024

I can confirm that this patch fixes my initial issue: the sm2/3 and svesm4 are not present on the V2 system that I am using. with this patch,

poetry run python
Python 3.10.12 (main, Nov 20 2023, 15:14:05) [GCC 11.4.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import archspec.cpu
>>> print (archspec.cpu.host())
neoverse_v2
>>> 

and neoverse_n1 without it.

@alalazo
Copy link
Member

alalazo commented May 24, 2024

Can you add a sample of the misidentified cpu to the tests? This would allow us to unit test this failed detection in archspec

@paolotricerri paolotricerri changed the title Address Issue 102 Remove optional SM crypto features from Neoverse V2 feature set May 28, 2024
@paolotricerri
Copy link
Contributor Author

@alalazo , just as a test, so will have to confirm with @dslarm properly, I have taken the archspec repo, removed the SM crypto flags from neoverse_v2 as well as from the linux-rhel9-neoverse_v2 test file and the function cpu.detect.host() returns a microarchitecture for neoverse_v2 because of the sorting function (and so the length of ancestors and features) and not neoverse_n1.

I'll confirm @dslarm 's test and try to add a similar test to the archspec repo.

@dslarm
Copy link

dslarm commented Jun 19, 2024

Can you add a sample of the misidentified cpu to the tests? This would allow us to unit test this failed detection in archspec

@alalazo - this is the flags excerpt from cpuinfo:

Flags:              fp asimd evtstrm aes pmull sha1 sha2 crc32 atomics fphp asim
                    dhp cpuid asimdrdm jscvt fcma lrcpc dcpop sha3 asimddp sha51
                    2 sve asimdfhm dit uscat ilrcpc flagm ssbs sb paca pacg dcpo
                    dp sve2 sveaes svepmull svebitperm svesha3 flagm2 frint svei
                    8mm svebf16 i8mm bf16 dgh rng bti

@paolotricerri
Copy link
Contributor Author

Hi @alalazo,
It should be safe to remove these flags from Neoverse V2 now because of the test on cpuid I propose in another PR.

What do you think?

Thanks
P

Copy link
Member

@alalazo alalazo left a comment

Choose a reason for hiding this comment

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

@paolotricerri I think using cpuid is the way to disambiguate. On #106 (comment), can you add a relevant sample of /proc/cpuinfo as done, for instance in:

?

@alalazo alalazo self-assigned this Jul 5, 2024
@paolotricerri
Copy link
Contributor Author

@alalazo , in the test folder I already see a file with data for the neoverse_V2 case. That example has got the feature that this patch is trying to remove. Do you mean adding another one taking the features from the comment above #106 (comment) ?

@alalazo
Copy link
Member

alalazo commented Jul 5, 2024

Do you mean adding another one taking the features from the comment above #106 (comment) ?

Yes, they would be both two real cases of neoverse_v2 that we need to detect.

@alalazo
Copy link
Member

alalazo commented Aug 6, 2024

@paolotricerri Can you add a sample of /proc/cpuinfo for the Neoverse V2 without the crypto features?

@alalazo alalazo changed the title Remove optional SM crypto features from Neoverse V2 feature set Neoverse V2: remove sm3, sm4 and svesm4 Aug 6, 2024
Copy link
Member

@alalazo alalazo left a comment

Choose a reason for hiding this comment

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

Another consideration: do we need to remove sm3 and sm4 also from noeverse-v1? I would assume so, but seeking confirmation @dslarm

@paolotricerri
Copy link
Contributor Author

paolotricerri commented Aug 7, 2024

I have moved the question I had over to #115 as that PR closes this one. Thanks

@alalazo alalazo closed this in #115 Aug 12, 2024
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.

Neoverse V2 detected as Neoverse N1
4 participants