[mlir][test] Make SME e2e tests require an emulator #86489
Conversation
Integration tests for ArmSME require an emulator (there's no hardware available). Disable the tests when an emulator is not available. I'm also adding a note in the docs for future reference.
banach-space
requested review from
dcaballe and
nicolasvasilache
as code owners
|
@llvm/pr-subscribers-mlir-sme @llvm/pr-subscribers-mlir-linalg Author: Andrzej Warzyński (banach-space) ChangesIntegration tests for ArmSME require an emulator (there's no hardware I'm also adding a note in the docs for future reference. Full diff: https://github.com/llvm/llvm-project/pull/86489.diff 3 Files Affected:
diff --git a/mlir/docs/Dialects/ArmSME.md b/mlir/docs/Dialects/ArmSME.md
index 7326150bcd1156..66f62d07a78545 100644
--- a/mlir/docs/Dialects/ArmSME.md
+++ b/mlir/docs/Dialects/ArmSME.md
@@ -14,6 +14,14 @@ integration tests for reference:
* [Linalg/CPU/ArmSME/matmul.mlir](https://github.com/llvm/llvm-project/blob/main/mlir/test/Integration/Dialect/Linalg/CPU/ArmSME/matmul.mlir)
* [Vector/CPU/ArmSME/test-outerproduct-f64.mlir](https://github.com/llvm/llvm-project/blob/main/mlir/test/Integration/Dialect/Vector/CPU/ArmSME/test-outerproduct-f64.mlir)
+In order to run ArmSME integration tests, include these flags in the CMake
+invokation when configuring LLVM and MLIR:
+```bash
+ -DMLIR_INCLUDE_INTEGRATION_TESTS=On
+ -DMLIR_RUN_ARM_SME_TESTS=On
+ -DARM_EMULATOR_EXECUTABLE=<path-to-emulator>
+```
+
These tests are run "post-commit" by the
[clang-aarch64-sve-vla](https://lab.llvm.org/buildbot/#/builders/197) LLVM
BuildBot worker.
diff --git a/mlir/test/Integration/Dialect/Linalg/CPU/ArmSME/lit.local.cfg b/mlir/test/Integration/Dialect/Linalg/CPU/ArmSME/lit.local.cfg
index 296b4419438e8a..083574b1af74d4 100644
--- a/mlir/test/Integration/Dialect/Linalg/CPU/ArmSME/lit.local.cfg
+++ b/mlir/test/Integration/Dialect/Linalg/CPU/ArmSME/lit.local.cfg
@@ -4,6 +4,10 @@ import sys
if not config.mlir_run_arm_sme_tests:
config.unsupported = True
+# With no hardware available, ArmSME tests require emulation.
+if not config.arm_emulator_executable:
+ config.unsupported = True
+
# No JIT on win32.
if sys.platform == "win32":
config.unsupported = True
diff --git a/mlir/test/Integration/Dialect/Vector/CPU/ArmSME/lit.local.cfg b/mlir/test/Integration/Dialect/Vector/CPU/ArmSME/lit.local.cfg
index 296b4419438e8a..083574b1af74d4 100644
--- a/mlir/test/Integration/Dialect/Vector/CPU/ArmSME/lit.local.cfg
+++ b/mlir/test/Integration/Dialect/Vector/CPU/ArmSME/lit.local.cfg
@@ -4,6 +4,10 @@ import sys
if not config.mlir_run_arm_sme_tests:
config.unsupported = True
+# With no hardware available, ArmSME tests require emulation.
+if not config.arm_emulator_executable:
+ config.unsupported = True
+
# No JIT on win32.
if sys.platform == "win32":
config.unsupported = True
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
✅ With the latest revision this PR passed the Python code formatter. |
| # With no hardware available, ArmSME tests require emulation. | ||
| if not config.arm_emulator_executable: | ||
| config.unsupported = True | ||
|
|
There was a problem hiding this comment.
I would rather have this be an error (inside CMake?): we shouldn't allow -DMLIR_RUN_ARM_SME_TESTS=On without the emulator.
Here the user will see ninja check-mlir complete successfully but ignoring these tests despite the CMake flag enabling them.
There was a problem hiding this comment.
Agreed, thanks! I've just updated this PR to use CMake instead.
|
Who is this change for? I don't see why we need to do anything here, if anything this is creating work for our future selves. |
Switch to using CMake instead
This is for anyone who'd like to try the SME e2e tests in MLIR :) The intention is to document things better - both through "actual" documentation and config scripts (with the latest version, CMake will complain if you set
I'm happy to address any future issues that this may bring 😅 Also, it's just 3 lines of CMake. |
I think this is introducing unnecessary guardrails that aren't required to try the e2e tests. I would think it's pretty obvious an emulator is required when a user hits SIGILLs when trying to run the tests and that's reason enough that this isn't required, but this is also removing an option other users may want. I've seen setups in the past for example where emulation is enabled opaquely via binfmt_misc on Linux.
No lines is better than 3 |
This change is trying to help folks for whom it would be less obvious :) And even for experts, a meaningful CMake error saying "you need to set-up this CMake variable" would be more informative compared to what we get today. |
This does not seem like a useful comment IMO. A nice error message at configuration time is much better than a compile time failure, which is itself much better than a link-time failure, which itself is much better than a runtime test failure! There is much more than "3 lines" in terms of complexity that exists in MLIR as safeguard against user mis-configuration. llvm-project/mlir/include/mlir/IR/OpDefinition.h Lines 2062 to 2068 in 80487e1 (same code exists in other places) Leading to: llvm-project/mlir/include/mlir/IR/Dialect.h Lines 231 to 243 in 80487e1
That seems like a valid use-case, @banach-space how do you see this supported? |
Apologies, that wasn't helpful.
I agree, hard to argue with that, failing early is a good thing.
You've touched on a further issue, if we had done this for SVE we wouldn't have been able to run the integration tests on hardware without removing it. However, we don't all have access to hardware at the same time, at what point do we decide this can be removed? Lit has constraints [1] which are the typically used for situations like this. The where [1] https://llvm.org/docs/TestingGuide.html#constraining-test-execution |
Apologies, that wasn't constructive. I can see the improvement you're trying to make and agree it's better to be pointed in the right direction. I'm just not sure about the current approach. |
|
Thanks for the feedback!
There's a couple of points here. 1. binfmt_misc on LinuxI think that in terms of these integration tests, "less is more" and we should try to avoid supporting too many scenarios. When the set of scenarios is limited and well defined, reproducing failures and helping folks fix their issues over "e-mail" is much easier. I also feel that we are making our job easier for ourselves if we clearly define what interfaces we'd like our users to use (e.g. 2. Hardware vs no-hardwareIt's not clear to me how to best support such cases. SVE is a great example - some folks have access to hardware, others don't (i.e. some people require an emulator, others don't). But this isn't really SVE specific, right? It's more about the overall infra that we have in MLIR. Today, there's no SME hardware, so the approach proposed here is valid. Once SME hardware is available, that won't be the case and that's when I'd revert this. Something longer term would be much better, I agree. LIT constraints
These constraints are "compile-time" rather than "run-time" constraints. For example, things like available targets (for stuff and: I don't see how we could use these here today (see below for an idea). AlternativesOption 1Ultimately, on unsupported hardware, it's Option 2One other option would involve updating CMake. There's LLVM_TARGET_ARCH and we could add // REQUIRES: (target=aarch64 AND target-arch-feature=sme) OR arm-emulator-executableI guess that this could scale nicely to other targets? Thoughts? |
The lit feature could be "support-sme" and would be a combination of "HW has native support" OR "the emulator is provided"? (I think it's a bit like your option 2, I would just make this a feature of its own in our lit config) |
Sure, but I wouldn't try mixing |
|
How do you query the HW support during the lit config phase though? |
The information would have come from the user. Here's what I'm proposing:
Setting Not ideal and not that automatic, but that's the best I have ATM. I don't know how to reliably query hardware, hence I am hesitant to explore that. Suggestions are very welcome though! |
|
I don't quite get the proposal, what is the meaning of LLVM_TARGET_ARCH_FEATURES? It does not seem to provide value to me to do so, it seems to me that the user is already explicit with
Have cmake compile an input file, run it, and check the result? |
There's an example in compiler-rt [1] checking if the host compiler supports SME, perhaps could use the same input/test but compile and run with CMake There's also the [1] llvm-project/compiler-rt/cmake/builtin-config-ix.cmake Lines 39 to 45 in 8d77d36 [2] https://cmake.org/cmake/help/latest/command/try_run.html#command:try_run [3] https://clang.llvm.org/docs/LanguageExtensions.html#builtin-cpu-supports [4] https://gcc.gnu.org/onlinedocs/gcc/x86-Built-in-Functions.html#index-_005f_005fbuiltin_005fcpu_005fsupports-1 [5] https://gcc.gnu.org/onlinedocs/gcc/AArch64-Built-in-Functions.html |
Adds two new CMake functions to query the host system: * `check_hwcap`, * `check_emulator`. Together, these functions are used to check whether a given set of MLIR integration tests require an emulator. If yes, then the corresponding CMake var that defies the required emulator executable is also checked. `check_hwcap` relies on ELF_HWCAP for discovering CPU features from userspace on Linux systems. This is the recommended approach for Arm CPUs running on Linux as outlined in this blog post: * https://community.arm.com/arm-community-blogs/b/operating-systems-blog/posts/runtime-detection-of-cpu-features-on-an-armv8-a-cpu Other operating systems (e.g. Android) and CPU architectures will most likely require some other approach. Right now these new hooks are only used for SVE and SME integration tests.
|
Thanks for the pointers Cullen, that was very helpful! I've re-implemented this PR - please check the most recent commit:
👍🏻 I have found a solution, but as hinted in the commit msg it's quite specific to Arm CPUs running on Linux. I can extend that to Android once/if required, but supporting other non-Arm hardware will require input from experts that actually know it 😅 |
mlir/test/CMakeLists.txt
Outdated
| return (hwcaps & <HWCAP_SPEC>) != 0; | ||
| } | ||
| ]====] | ||
| ) |
There was a problem hiding this comment.
I thought you'd write some inline ASM performing a simple SVE operation and check that it does not crash, so that you wouldn't depend on any header.
I'm fine with this though.
There was a problem hiding this comment.
I thought you'd write some inline ASM performing a simple SVE operation and check that it does not crash, so that you wouldn't depend on any header.
I'm fine with this though.
Likewise, perhaps more portable that way, but I can see merits in this approach as well.
There was a problem hiding this comment.
- I wanted to use something that's documented - I came across HWCAP and I really like the end result.
- I want to avoid distributing and running code that might crash. IMHO, if there's a solution that avoids that then that's better :)
c-rhodes
left a comment
c-rhodes
left a comment
There was a problem hiding this comment.
Left a few minor comments but also LGTM, cheers!
mlir/test/CMakeLists.txt
Outdated
| return (hwcaps & <HWCAP_SPEC>) != 0; | ||
| } | ||
| ]====] | ||
| ) |
There was a problem hiding this comment.
I thought you'd write some inline ASM performing a simple SVE operation and check that it does not crash, so that you wouldn't depend on any header.
I'm fine with this though.
Likewise, perhaps more portable that way, but I can see merits in this approach as well.
Move new CMake logic to a seperate file, fix typos
This reverts commit 7b52552. Broken bot: * https://lab.llvm.org/buildbot/#/builders/179/builds/9794
Adds two new CMake functions to query the host system: * `check_hwcap`, * `check_emulator`. Together, these functions are used to check whether a given set of MLIR integration tests require an emulator. If yes, then the corresponding CMake var that defies the required emulator executable is also checked. `check_hwcap` relies on ELF_HWCAP for discovering CPU features from userspace on Linux systems. This is the recommended approach for Arm CPUs running on Linux as outlined in this blog post: * https://community.arm.com/arm-community-blogs/b/operating-systems-blog/posts/runtime-detection-of-cpu-features-on-an-armv8-a-cpu Other operating systems (e.g. Android) and CPU architectures will most likely require some other approach. Right now these new hooks are only used for SVE and SME integration tests. This relands #86489 with the following changes: * Replaced: `set(hwcap_test_file ${CMAKE_BINARY_DIR}/${CMAKE_FILES_DIRECTORY}/hwcap_check.c)` with: `set(hwcap_test_file ${CMAKE_BINARY_DIR}/temp/hwcap_check.c)` The former would trigger an infinite loop when running `ninja` (after the initial CMake configuration). * Fixed commit msg. Previous one was taken from the initial GH PR commit rather than the final re-worked solution (missed this when merging via GH UI). * A couple more NFCs/tweaks.
5 participants
Integration tests for ArmSME require an emulator (there's no hardware
available). Make sure that CMake complains if
MLIR_RUN_ARM_SME_TESTSis set while
ARM_EMULATOR_EXECUTABLEis empty.I'm also adding a note in the docs for future reference.