[lldb][AArch64] Add isAArch64SMEFA64 check to SME testing #68094
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
FEAT_SME_FA64 (smefa64 in Linux cpuinfo) allows the use of the full A64 instruction set while in streaming SVE mode. See https://developer.arm.com/documentation/ddi0616/latest/ for details. This means for example if we want to write to the ffr register during or use floating point registers while in streaming mode, we need this extension. I initially was using QEMU which has it by default, and switched to Arm's FVP which does not. So this change adds a more strict check and converts most of the tests to use that. It would be possible in some cases to avoid the offending instructions but it would be a lot of effort and liable to fail randomly as the C library changes. It is also my assumption that the majority of systems will have smefa64 as QEMU has chosen to have. If I turn out to be wrong, we can make the effort to get the tests working without smefa64.
|
@llvm/pr-subscribers-lldb ChangesFEAT_SME_FA64 (smefa64 in Linux cpuinfo) allows the use of the full A64 instruction set while in streaming SVE mode. See https://developer.arm.com/documentation/ddi0616/latest/ for details. This means for example if we want to write to the ffr register during or use floating point registers while in streaming mode, we need this extension. I initially was using QEMU which has it by default, and switched to Arm's FVP which does not. So this change adds a more strict check and converts most of the tests to use that. It would be possible in some cases to avoid the offending instructions but it would be a lot of effort and liable to fail randomly as the C library changes. It is also my assumption that the majority of systems will have smefa64 as QEMU has chosen to have. If I turn out to be wrong, we can make the effort to get the tests working without smefa64. Full diff: https://github.com/llvm/llvm-project/pull/68094.diff 8 Files Affected:
diff --git a/lldb/packages/Python/lldbsuite/test/lldbtest.py b/lldb/packages/Python/lldbsuite/test/lldbtest.py
index c8670b208ec3f0c..2f4130d3ce68ae0 100644
--- a/lldb/packages/Python/lldbsuite/test/lldbtest.py
+++ b/lldb/packages/Python/lldbsuite/test/lldbtest.py
@@ -1271,6 +1271,12 @@ def isAArch64SVE(self):
def isAArch64SME(self):
return self.isAArch64() and "sme" in self.getCPUInfo()
+ def isAArch64SMEFA64(self):
+ # smefa64 allows the use of the full A64 instruction set in streaming
+ # mode. This is required by certain test programs to setup register
+ # state.
+ return self.isAArch64SME() and "smefa64" in self.getCPUInfo()
+
def isAArch64MTE(self):
return self.isAArch64() and "mte" in self.getCPUInfo()
diff --git a/lldb/test/API/commands/register/register/aarch64_dynamic_regset/TestArm64DynamicRegsets.py b/lldb/test/API/commands/register/register/aarch64_dynamic_regset/TestArm64DynamicRegsets.py
index 2fb8b33126417c2..0ad69c268a9fd29 100644
--- a/lldb/test/API/commands/register/register/aarch64_dynamic_regset/TestArm64DynamicRegsets.py
+++ b/lldb/test/API/commands/register/register/aarch64_dynamic_regset/TestArm64DynamicRegsets.py
@@ -142,8 +142,8 @@ def make_za_value(self, vl, generator):
def test_aarch64_dynamic_regset_config_sme(self):
"""Test AArch64 Dynamic Register sets configuration, but only SME
registers."""
- if not self.isAArch64SME():
- self.skipTest("SME must be present.")
+ if not self.isAArch64SMEFA64():
+ self.skipTest("SME and the smefa64 extension must be present")
register_sets = self.setup_register_config_test("sme")
diff --git a/lldb/test/API/commands/register/register/aarch64_sve_registers/rw_access_dynamic_resize/TestSVEThreadedDynamic.py b/lldb/test/API/commands/register/register/aarch64_sve_registers/rw_access_dynamic_resize/TestSVEThreadedDynamic.py
index 8bcb76776459d01..b19039f0b5212b4 100644
--- a/lldb/test/API/commands/register/register/aarch64_sve_registers/rw_access_dynamic_resize/TestSVEThreadedDynamic.py
+++ b/lldb/test/API/commands/register/register/aarch64_sve_registers/rw_access_dynamic_resize/TestSVEThreadedDynamic.py
@@ -108,8 +108,9 @@ def run_sve_test(self, mode):
if (mode == Mode.SVE) and not self.isAArch64SVE():
self.skipTest("SVE registers must be supported.")
- if (mode == Mode.SSVE) and not self.isAArch64SME():
- self.skipTest("Streaming SVE registers must be supported.")
+ if (mode == Mode.SSVE) and not self.isAArch64SMEFA64():
+ self.skipTest("Streaming SVE registers must be supported and the "
+ "smefa64 extension must be present.")
self.build_for_mode(mode)
@@ -201,8 +202,9 @@ def test_ssve_registers_dynamic_config(self):
def setup_svg_test(self, mode):
# Even when running in SVE mode, we need access to SVG for these tests.
- if not self.isAArch64SME():
- self.skipTest("Streaming SVE registers must be present.")
+ if not self.isAArch64SMEFA64():
+ self.skipTest("Streaming SVE registers must be present and the "
+ "smefa64 extension must be present.")
self.build_for_mode(mode)
diff --git a/lldb/test/API/commands/register/register/aarch64_sve_registers/rw_access_static_config/TestSVERegisters.py b/lldb/test/API/commands/register/register/aarch64_sve_registers/rw_access_static_config/TestSVERegisters.py
index 82b79b8d4b6cc2b..ac99652442b5ddd 100644
--- a/lldb/test/API/commands/register/register/aarch64_sve_registers/rw_access_static_config/TestSVERegisters.py
+++ b/lldb/test/API/commands/register/register/aarch64_sve_registers/rw_access_static_config/TestSVERegisters.py
@@ -85,8 +85,9 @@ def skip_if_needed(self, mode):
if (mode == Mode.SVE) and not self.isAArch64SVE():
self.skipTest("SVE registers must be supported.")
- if (mode == Mode.SSVE) and not self.isAArch64SME():
- self.skipTest("SSVE registers must be supported.")
+ if (mode == Mode.SSVE) and not self.isAArch64SMEFA64():
+ self.skipTest("SSVE registers must be supported and the smefa64 "
+ "extension must be present.")
def sve_registers_configuration_impl(self, mode):
self.skip_if_needed(mode)
diff --git a/lldb/test/API/commands/register/register/aarch64_sve_simd_registers/TestSVESIMDRegisters.py b/lldb/test/API/commands/register/register/aarch64_sve_simd_registers/TestSVESIMDRegisters.py
index 814ca98369fca57..def93c78abc2745 100644
--- a/lldb/test/API/commands/register/register/aarch64_sve_simd_registers/TestSVESIMDRegisters.py
+++ b/lldb/test/API/commands/register/register/aarch64_sve_simd_registers/TestSVESIMDRegisters.py
@@ -41,8 +41,9 @@ def skip_if_needed(self, mode):
if (mode == Mode.SVE) and not self.isAArch64SVE():
self.skipTest("SVE registers must be supported.")
- if (mode == Mode.SSVE) and not self.isAArch64SME():
- self.skipTest("SSVE registers must be supported.")
+ if (mode == Mode.SSVE) and not self.isAArch64SMEFA64():
+ self.skipTest("SSVE registers must be supported and the smefa64 "
+ "extension must be present.")
def make_simd_value(self, n):
pad = " ".join(["0x00"] * 7)
diff --git a/lldb/test/API/commands/register/register/aarch64_za_register/za_dynamic_resize/TestZAThreadedDynamic.py b/lldb/test/API/commands/register/register/aarch64_za_register/za_dynamic_resize/TestZAThreadedDynamic.py
index 65d1071c26b2a34..884340b395a448d 100644
--- a/lldb/test/API/commands/register/register/aarch64_za_register/za_dynamic_resize/TestZAThreadedDynamic.py
+++ b/lldb/test/API/commands/register/register/aarch64_za_register/za_dynamic_resize/TestZAThreadedDynamic.py
@@ -65,8 +65,10 @@ def check_disabled_za_register(self, svg):
self.expect("register read za", substrs=[self.gen_za_value(svg, lambda r: 0)])
def za_test_impl(self, enable_za):
- if not self.isAArch64SME():
- self.skipTest("SME must be present.")
+ # Although the test program doesn't obviously do any operations that
+ # would need smefa64, calls to libc functions like memset may do.
+ if not self.isAArch64SMEFA64():
+ self.skipTest("SME and the sm3fa64 extension must be present")
self.build()
supported_vg = self.get_supported_vg()
diff --git a/lldb/test/API/commands/register/register/aarch64_za_register/za_dynamic_resize/main.c b/lldb/test/API/commands/register/register/aarch64_za_register/za_dynamic_resize/main.c
index fd2590dbe411f7f..05839c26336cc8e 100644
--- a/lldb/test/API/commands/register/register/aarch64_za_register/za_dynamic_resize/main.c
+++ b/lldb/test/API/commands/register/register/aarch64_za_register/za_dynamic_resize/main.c
@@ -29,6 +29,7 @@ void set_za_register(int svl, int value_offset) {
// you have. So setting one that didn't exist would actually set one that did.
// That's why we need the streaming vector length here.
for (int i = 0; i < svl; ++i) {
+ // This may involve instructions that require the smefa64 extension.
memset(data, i + value_offset, MAX_VL_BYTES);
// Each one of these loads a VL sized row of ZA.
asm volatile("mov w12, %w0\n\t"
diff --git a/lldb/test/API/commands/register/register/aarch64_za_register/za_save_restore/TestZARegisterSaveRestore.py b/lldb/test/API/commands/register/register/aarch64_za_register/za_save_restore/TestZARegisterSaveRestore.py
index 910966a0b3b0bc5..a647c91f71119ec 100644
--- a/lldb/test/API/commands/register/register/aarch64_za_register/za_save_restore/TestZARegisterSaveRestore.py
+++ b/lldb/test/API/commands/register/register/aarch64_za_register/za_save_restore/TestZARegisterSaveRestore.py
@@ -106,8 +106,8 @@ def check_za_disabled(self, vl):
self.expect("register read za", substrs=[self.make_za_value(vl, lambda row: 0)])
def za_expr_test_impl(self, sve_mode, za_state, swap_start_vl):
- if not self.isAArch64SME():
- self.skipTest("SME must be present.")
+ if not self.isAArch64SMEFA64():
+ self.skipTest("SME and the smefa64 extension must be present.")
supported_svg = self.get_supported_svg()
if len(supported_svg) < 2:
|
|
✅ With the latest revision this PR passed the Python code formatter. |
omjavaid
reviewed
Oct 6, 2023
There was a problem hiding this comment.
After this change isAArch64SME check will be replaced on all locations?
So the manual says "The SVE FFR predicate register is not architecturally visible when the PE is in Streaming SVE mode if FEAT_SME_FA64 is not implemented or not enabled at the current Exception level."
All SVE tests seem to use FFR does it mean isAArch64SME is now redundant as far as LLDB testing is concerned?
|
I ran all the tests that originally had The ffr use you specifically noticed, the tests use isAArch64SMEFA64 up front and skip if it's not there. Then later when they want an SVCR value they just use isAArch64SME since strictly that's all that bit needs to know. We could just have isAArch64SMEFA64 everywhere even if we didn't really need smefa64, but I figured it would be nice to have as many tests run in either situation as we can (for my benefit more than anything else right now). |
omjavaid
approved these changes
Oct 20, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
FEAT_SME_FA64 (smefa64 in Linux cpuinfo) allows the use of the full A64 instruction set while in streaming SVE mode.
See https://developer.arm.com/documentation/ddi0616/latest/ for details.
This means for example if we want to write to the ffr register during or use floating point registers while in streaming mode, we need this extension.
I initially was using QEMU which has it by default, and switched to Arm's FVP which does not. So this change adds a more strict check and converts most of the tests to use that. It would be possible in some cases to avoid the offending instructions but it would be a lot of effort and liable to fail randomly as the C library changes.
It is also my assumption that the majority of systems will have smefa64 as QEMU has chosen to have. If I turn out to be wrong, we can make the effort to get the tests working without smefa64.
isAArch64SMEremains for some tests, which are as follows:test_aarch64_dynamic_regset_configmerely checks for the presence of a register set, which appears for any SME system not just one with smefa64.test_aarch64_dynamic_regset_config_sme_za_disabledonly needs the ZA register and does not enter streaming mode.test_sme_not_presenttests for the absence of the SME register set, so must be skipped if any form of SME is present.TestSVERegisters.pyneed to know if SME is present at all to generate an expected SVCR value. Earlier in the callstack something else checkedisAArch64SMEFA64already.TestAArch64LinuxTLSRegisters.pyneeds to test thetpidr2register if any form of SME is present. msr/mrs instructions are used to do this and are allowed even if smefa64 is not present.