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

Add AArch64 SVE2 Vector Length Agnostic builders #317

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

DavidSpickett
Copy link
Contributor

These generate code that uses the SVE2 extension, as opposed to our existing builders that only use SVE. Therefore they will run on our Graviton 4 workers only, which are Neoverse v2 cores with SVE and SVE2.

The optimisation guide:
https://github.com/aws/aws-graviton-getting-started/blob/main/c-c++.md

Suggests -mcpu=neoverse-v2 for best performance, but here I'm using it just so we get the SVE2 feature enabled. If we used the balanced -mcpu=neoverse-512tvb, we wouldn't get SVE2 without fiddly extra flags.

$ ~/clang+llvm-18.1.8-aarch64-linux-gnu/bin/clang /tmp/test.c -o /dev/null -mcpu=neoverse-v2 -###
<...> "-target-feature" "+sve" "-target-feature" "+sve2-bitperm" "-target-feature" "+sve2" <...>

I have not added vector length settings as these builders will generate scalable ("length agnostic" - LA) code.

Otherwise, the builders have identical setups to the existing SVE VLA builders.

Aside from the new CPU and different core/RAM amounts, these
will be the same as our existing Gravition 3 ("g3") workers.

Not adding SVE VLS at this point to keep disk usage reasonable
and hedge against any stability issues.
These generate code that uses the SVE2 extension, as opposed
to our existing builders that only use SVE. Therefore they will
run on our Graviton 4 workers only, which are Neoverse v2 cores
with SVE and SVE2.

The optimisation guide:
https://github.com/aws/aws-graviton-getting-started/blob/main/c-c++.md

Suggests -mcpu=neoverse-v2 for best performance, but here I'm using
it just so we get the SVE2 feature enabled. If we used the balanced
-mcpu=neoverse-512tvb, we wouldn't get SVE2 without fiddly extra flags.

```
$ ~/clang+llvm-18.1.8-aarch64-linux-gnu/bin/clang /tmp/test.c -o /dev/null -mcpu=neoverse-v2 -###
<...> "-target-feature" "+sve" "-target-feature" "+sve2-bitperm" "-target-feature" "+sve2" <...>
```

I have not added vector length settings as these builders will
generate scalable ("length agnostic" - LA) code.

Otherwise, the builders have identical setups to the existing SVE
VLA builders.
@DavidSpickett
Copy link
Contributor Author

The first 2 commits of this are #311. Please review only the last commit here.

@maxim-kuvyrkov please confirm that this is exactly what was requested.

@DavidSpickett
Copy link
Contributor Author

I've also confirmed that at least right now, these configurations are green. They will be silent to begin with anyway, so that we can work out any capacity issues.

Copy link
Contributor

@gkistanova gkistanova left a comment

Choose a reason for hiding this comment

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

LGTM.

Copy link
Contributor

@maxim-kuvyrkov maxim-kuvyrkov left a comment

Choose a reason for hiding this comment

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

Other than the parallelism comment -- looks great. Thanks!

},
testsuite_flags=[
'--cppflags', '-mcpu=neoverse-v2 -mllvm -scalable-vectorization=preferred -mllvm -treat-scalable-fixed-error-as-warning=false -O3',
'--threads=32', '--build-threads=32'],
Copy link
Contributor

Choose a reason for hiding this comment

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

The current instances have 48 cores and 4GB RAM per core, so we should be OK with max parallelism of 48. Same comment below for 2stage builder.

@@ -586,8 +586,7 @@
"-DCMAKE_CXX_FLAGS='-mcpu=neoverse-v2'",
"-DLLVM_ENABLE_LLD=True",
"-DMLIR_INCLUDE_INTEGRATION_TESTS=True",
"-DMLIR_RUN_ARM_SVE_TESTS=True",
"-DLLVM_LIT_ARGS='-v'"])},
Copy link
Contributor

Choose a reason for hiding this comment

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

[Just checking] Will you remove "-v" from the existing configurations in a separate PR?

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.

4 participants