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

Rename graviton products with ARM uarch name #57

Merged
merged 6 commits into from
Oct 17, 2022

Conversation

alalazo
Copy link
Member

@alalazo alalazo commented Oct 9, 2022

fixes #41

Modifications:

  • Change the name of the Graviton products to corresponding generic ARM uarch names
  • Add generic levels for the ARM architectures
  • Rename test files accordingly

@alalazo alalazo changed the title Rename graviton product with ARM uarch name Rename graviton products with ARM uarch name Oct 9, 2022
@alalazo alalazo requested a review from tgamblin October 9, 2022 10:18
@alalazo

This comment was marked as outdated.

@alalazo alalazo added the enhancement New feature or request label Oct 9, 2022
],
"apple-clang": [
{
"versions": ":",
Copy link
Member Author

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"
Copy link
Member

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

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?

Copy link
Member Author

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.

@alalazo alalazo requested a review from boegel October 10, 2022 07:12
tgamblin
tgamblin previously approved these changes Oct 12, 2022
Copy link
Member

@tgamblin tgamblin left a 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.

@alalazo
Copy link
Member Author

alalazo commented Oct 17, 2022

@boegel Can you re-review? Is there something missing?

Copy link
Member

@boegel boegel left a 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...

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

Successfully merging this pull request may close these issues.

Renaming AWS graviton2 target as neoverse-n1
3 participants