cpu: aarch64: Add sve isa enum with no vlen #4475
Conversation
jondea
force-pushed
the
add-sve-isa-with-no-vlen
branch
from
5ff7ead to
9baaba6
Compare
src/cpu/aarch64/cpu_isa_traits.hpp
Outdated
| dnnl_cpu_isa_sve_128 = 0x7, | ||
| /// AArch64 SVE 256 bits | ||
| dnnl_cpu_isa_sve_256 = 0xf, | ||
| /// AArch64 SVE 512 bits |
There was a problem hiding this comment.
Changing the enum values will break the ABI right? Also isn't removing vlen-384 technically backwards incompatible? (even if no one is using it)
There was a problem hiding this comment.
As far as I can tell, this enum is internal only, so shouldn't affect the API/ABI
There was a problem hiding this comment.
Nope! You are correct, the comment above this enum is wrong. Even though there is no API for dnnl_cpu_isa_t on AArch64, these do actually affect the ABI via get_effective_cpu_isa, that's why the unit tests are failing. Interestingly, the unit tests also look wrong as they are comparing x64 and AArch64 isa types (e.g. https://github.com/uxlfoundation/oneDNN/blob/main/tests/gtests/graph/unit/backend/dnnl/test_convolution.cpp#L3841).
I think the fix for this is just leave sve as an internal only enum value.
There was a problem hiding this comment.
I have changed this patch to be more conservative, I don't think it changes any aspect of the external API/ABI now.
Add `sve` to the `cpu_isa_t` enum, so that we can express that an implementation does not depend on vector length at compile time, but still use a template parameter to separate `asimd` and `sve`. It is semantically different to `sve_128` but almost functionally equivalent, the exception being that trying to access the vlen at compile time is a compilation error. This forces the implementation to use a runtime value for the vlen, eliminating a potential logical error and making the intent clearer. Also removes sve_384, because SVE length must be a power of 2 https://developer.arm.com/documentation/102476/0101/Introducing-SVE Note that this is for internal use, we are not adding a new dnnl_cpu_isa or a new value for MAX_CPU_ISA, so there is no API or ABI change.
Clean up use of SIMD width/bytes in jit_uni_eltwise (including one example where we were referring to cacheline as SIMD width, which is not necessarily equivalent)
jondea
force-pushed
the
add-sve-isa-with-no-vlen
branch
from
9baaba6 to
0ef4fd3
Compare
4 participants
Description
Add
sveto thecpu_isa_tenum for AArch64, so that we can express that an implementation is vector length agnostic (VLA at kernel generation, not yet using VLA instructions, but we can work towards that). It is semantically different tosve_128but almost functionally equivalent, the exception being that trying to access the vlen at compile time is a compilation error. This forces the implementation to use a runtime value for the vlen, elliminating a potential logical error. Add/modify a few helper functions which allow kernel generators to access runtime value generically in the correct way.The functionality is demonstrated by its use in jit_uni_eltwise.
Broadly speaking, there should be no functional changes here, this is more a code quality/dev improvement.
Also removes sve_384, because SVE length must be a power of 2
Checklist
General
make testandmake test_benchdnn_*) pass locally for each commit?