-
Notifications
You must be signed in to change notification settings - Fork 35
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
Rename graviton products with ARM uarch name #57
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
], | ||
"apple-clang": [ | ||
{ | ||
"versions": ":", |
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.
The versions for both arm
and apple-clang
need to be checked, but I guess we can do that in a follow up PR. The versions for gcc
and clang
should be correct, but maybe stricter than necessary - they can also be refined later.
}, | ||
{ | ||
"versions": "10.0:", | ||
"flags" : "-march=armv8.4-a+crypto+rcpc+sha3+sm4+sve+rng+ssbs+i8mm+bf16+nodotprod -mtune=neoverse-v1" |
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.
Why drop the -march
part?
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 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 it's worth adding that link somewhere, maybe in the README file in this repo?
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.
Sure, we might start another doc related PR and add more generally a section on "resources" to look at when adding a new compiler.
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 LGTM. We likely need to add aliases either on the spack side or within archspec
python, so that the binaries continue to work.
@boegel Can you re-review? Is there something missing? |
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.
No strong objections for my side.
This will obviously impact projects which are currently assuming that archspec will produce values like graviton2
rather than neoverse_n1
, so we should make it very clear in the release notes for the next archspec
release that includes these changes...
fixes #41
Modifications: