feat: Single ISA CMake build #1212
Conversation
| make("QueryWeightFormat", {arm_compute::WeightFormat::OHWIo8i4_bf16}))) | ||
| { | ||
| if (Scheduler::get().cpu_info().has_bf16() && (arm_gemm::utils::get_vector_length<float>() == 8)) | ||
| if (Scheduler::get().cpu_info().has_bf16() && Scheduler::get().cpu_info().has_sve() && |
There was a problem hiding this comment.
Why did we need to change here?
There was a problem hiding this comment.
It was causing illegal instruction exception in Fixed Format Kernel tests if not guarded with has_sve
cmake/compilers/setup.cmake
Outdated
| ) | ||
|
|
||
| else() | ||
| if(ACL_ARCH_ISA STREQUAL "arm64-v8a") |
There was a problem hiding this comment.
I think we should cover other synonymus options in these if/else's, such as armv8a here.
In essence, we should cover everything mentioned in Sconstruct here:
EnumVariable("arch", "Target Architecture. The x86_32 and x86_64 targets can only be used with neon=0 and opencl=1.", "armv7a",
allowed_values=("armv7a", "armv7a-hf", "arm64-v8a", "arm64-v8.2-a", "arm64-v8.2-a-sve", "arm64-v8.2-a-sve2", "x86_32", "x86_64",
"armv8a", "armv8.2-a", "armv8.2-a-sve", "armv8.6-a", "armv8.6-a-sve", "armv8.6-a-sve2", "armv8.6-a-sve2-sme2", "armv8r64", "
The goal is being able to call CMake from Scons and obtain the same behaviour.
There was a problem hiding this comment.
Sure, I can look into this however the design doc specifically mentioned three presets: arm64-v8a, arm64-v8.2-a, arm64-v8.6-a.
There was a problem hiding this comment.
Actually, let's rethink this. We can ignore 32-bit and armv7a ones as we're doing this for macOS in the scope of this epic. Same goes for armv8r64, x86 etc.
One simple thing to do is to get rid of the "64" bit in the arch names. So, CMake should take armv8-a, armv8.2-a etc. without the 64 in them. When calling from Scons (in the future), we can convert all of them to CMake compatible ones.
We should also be aware any of the misdesigns in the previous implementation (i.e. Scons) and address properly as we have the chance now. One of those misdesigns is to explicitly spell out each arch and feature combination in the supported archs, such as armv8.2-a-sve, armv8.6-a-sve, armv8.6-a-sve2-sme2. arch and feature are combined in the same argument and there are combinations that we can support but don't, and for each one of them we have to add a parameter option. I think we can and should do things differently when building the single-isa solution in CMake. So, I'm taking my initial suggestion back and revising it. How about we get the feature names above (we can start with sve, sve2 and sme2) as a comma separated list (or whatever you think is better), set the arch based on that and add the flags.
There was a problem hiding this comment.
Good suggestion. Does this mean that we do not need arch as input at all and instead deduce it based on feature names?
There was a problem hiding this comment.
Commenting here for completeness, we won't be decuding arch from the features. My last sentence in the above logn comment is a bit unfortunate :) I'm sorry for the confusion I caused. As agreed offline, we'll still specify the base arch. but take the feature list in some way (e.g. comma-separated) and build the -march compiler argument based on that. This will ensure scalability in the future.
wajahat-abbas
force-pushed
the
pr/single-isa
branch
from
16b0b9b to
ba7b052
Compare
Resolves: COMPMID-8547 Change-Id: I82d3e74b28284a4052a2042f47d2eff896cc9c38 Signed-off-by: Syed Wajahat Abbas Naqvi <syedwajahatabbas.naqvi@arm.com>
wajahat-abbas
force-pushed
the
pr/single-isa
branch
from
ba7b052 to
8c6390c
Compare
Resolves: COMPMID-8547
Change-Id: I82d3e74b28284a4052a2042f47d2eff896cc9c38
Signed-off-by: Syed Wajahat Abbas Naqvi syedwajahatabbas.naqvi@arm.com