Preliminary PR to merge ARM SVE branch #315
Conversation
|
I've created release package for testing this w/o the 'maintainer mode' requirements. See there: https://github.com/rdolbeau/fftw3/releases/tag/sve-test-release-001 |
|
@stevengj @matteo-frigo Can you comment on this? In particular the "used fixed-width implementation and enable all those of equal-or-narrower to the hardware width". It's a bit hackish but it's the best we have at this time. RISC-V is using the same principle for now. |
|
I would like to encourage the FFTW folks to consider this PR so it can be included (after some cleanup) into the next FFTW release. At this stage, my main interest is only about ARM SVE.
Though SVE vector length can scale from 128 to 2048 bits (via 128 bits increments), only 3 sizes are available today (128, 256 and 512. Fujitsu A64fx does not support 384 bits). So even if vector length agnostic FFTW might be achievable, I do not see this as mandatory because of the currently limited available vector lengths and the fact VLA code is (generally) slightly slower than vector length specific code. If needed, I can provide an option to set a maximum supported vector length in order to reduce the size of the FFTW library. Bottom line, I approve the approach of the PR and hope it can make its way upstream. |
Also, in its current incarnation SVE doesn't allow for non-power-of-2 multiple of 128 bits anymore, only power-of-2 multiple of 128 bit are. This is visible for instance in DDI0487J for the register ZCR_EL1: So this implementation should cover all cases, and I agree with you that 1024 and 2048 bits are currently only for emulators like QEmu and could (should?) be optional. 512 and 256 could also be optional for site-specific deployment. |
|
Done some minor clean-ups, minor improvement, and rebase to current HEAD. @ggouaillardet if you have the time to confirm this is still OK for you |
|
I've created a new package with pre-generated files, so maintainer mode is not required to test this on Arm+SVE, see https://github.com/rdolbeau/fftw3/releases/tag/sve-test-release-002 |
|
Thanks @rdolbeau and my apologies for the late reply. I noted |
Probably with all compilers that do not enable SVE by default (all of them, probably!), currently configure doesn't enable it explicitly so it has to be done in {C/CXX/F}FLAGS.
Good idea. I'll try that and add it to the PR ASAP. |
I can see performance benefits by extending to 256 bits and above. Do you happen to have performance data for the sve version comparing to the SIMD NEON version? |
|
Here is some performance number I collected a while ago for
Feel free to suggest other/better benchmarks if appropriate. |
|
Hello, I ran few experiments using the included benchmark Where
Baseline stands for the current release 3.3.10 built with We can observe significant gain on all SVE machines (A64FX, Neoverse V1 and V2), as well as no degradation on NEON only machine (Neoverse N1). Best regards. |
|
These results are very exciting. FFTW performance is extremely important to users of NVIDIA Grace (Neoverse V2), which is now the dominant architecture on the Green500. A great many people would be glad to see this commit in the next FFTW release. |
| @@ -0,0 +1,84 @@ | |||
| #include <stdio.h> | |||
| #include <string.h> | |||
| #include <stdlib.h> | |||
There was a problem hiding this comment.
You gate the include later in this file, so remove this
| unsigned int size = rp2(osize); | ||
| if (osize != size) |
There was a problem hiding this comment.
Not really important, but checking for a power of two can be done with:
if (!(osize & (osize - 1))
just a nice tidbit from Hacker's Delight
simd-support/simd-maskedsve.h
Outdated
There was a problem hiding this comment.
This is not to be included directly (accessed through, e.g. simd-maskedsve1024.h), so should it be named something else, e.g. simd-maskedsve.h.template?
and add simd-support/{generate_vtw.sh,vtw.h} into the dist tarball
ignore all files automatically generated for SVE support
ADD/SUB/MUL are three-addresses in SVE, but the masked form is only two-adresses. And there's a lot of reuse in FFTW3 (and complex arithmetic). But ACLE/SVE (i.e. intrinsics) don't have the non-masked form :-( So used inline ASM for force the non-masked version to be used. Masked-out lanes should be mostly zero, and are never stored anyway, so computing on them should be fine. This one will be reversed if it's not a performance win.
…mpiler/hardware dependent, and more tests are needed before settling on some defaults.
When configure'd with --enable-sve, try to build a sample SVE program and abort on failure, otherwise configure successes but make will fail.
|
I've created a new package with pre-generated files, so maintainer mode is not required to test this on Arm+SVE, see https://github.com/rdolbeau/fftw3/releases/tag/sve-test-release-003 |
|
@stevengj @matteo-frigo any comment on this? Is this OK to think about merging ? |
This is primarily for discussion ad probably need some cleaning up.
The current scheme using SVE (and RISC-V V) doesn't leverage the scalability; instead, it produces sets of codelets for all possible power-of-2 sizes, using masking. Only sets of a size <= to the hardware implementation width are enabled. See here.
It also auto-generate the simd-support/vtw.h file that defines the various VTW macro. This is useful as while SVE is limited to 2048 bits wide vector, RISC-V V is more or less unbounded and there's a least a 16384 bits wide implementation being developed.
So using SVE (or V) creates a fairly large, though versatile, library.
I has been tested in QEmu; using the Arm instruction Emulator; and on real hardware (Fujitsu A64FX, AWS Graviton 3).