SUMMARY Writing a test which relies on specific Arm 64 v8.x CPU feature(s) will fail if the .vgtest file cannot run tests/arm64_features to establish feature availability. STEPS TO REPRODUCE 1. Write a test which uses v8.2 half precision ops 2. Add 'test -x my_fp16_arithmetic_tests && ../../../tests/arm64_features fphp' 3. Run 'make regtest'. OBSERVED RESULT -- Running tests in none/tests/arm64 ---------------------------------- /bin/sh: ../../../tests/arm64_features: No such file or directory prereq returned 127: test -x my_fp16_arithmetic_tests && ../../../tests/arm64_features fphp EXPECTED RESULT The prerequisite should successfully call ../../../tests/arm64_features in order to run the test.
Created attachment 123538 [details] Patch implements tests/arm64_features.c Patch implements tests/arm64_features.c for future additions to prereq none/tests/arm64/* Version: valgrind-3.16.0.GIT (based on Git development head SHA 78054a6c0) OS: Linux The regression tests in none/tests/arm64 should check for existence of v8.2 and SVE hardware opcode features, in order to accommodate varying CPU versions.
Created attachment 133311 [details] Patch adds arm64 feature detection for regression tests This patch implements arm64 ISA features/attributes checks for regression tests, and must be committed after https://bugs.kde.org/show_bug.cgi?id=414268 The simple test of a v8.2 encoding shows how the fix for https://bugs.kde.org/show_bug.cgi?id=414268 can be used in IR creation on different arm64 versions. When both patches are committed, 'make regtest' should result in the following for v8.0 and 8.1: - - - snip fp_and_simd: valgrind -q ./fp_and_simd fp_and_simd_v82: (skipping, prereq failed: test -x fp_and_simd_v82 && ../../../tests/arm64_features fphp) integer: valgrind -q ./integer - - - snip The test should run and pass on v8.2: - - - snip fp_and_simd: valgrind -q ./fp_and_simd fp_and_simd_v82: valgrind -q ./fp_and_simd_v82 integer: valgrind -q ./integer - - - snip
Created attachment 133667 [details] Adds arm64 feature detection for regression tests
Created attachment 133751 [details] Adds arm64 feature detection for regression tests Removed v8.x version detection. Now using features only.
Assad, am I right to understand that the comment 3 patch has been made redundant by the comment 4 patch?
> Assad, am I right to understand that the comment 3 patch > has been made redundant by the comment 4 patch? That's correct Julian. I should have deleted the comment 3 patch when I uploaded the latest comment 4 patch. Can't seem to delete it after the comment 4 patch upload.
You can get to an "patch is obsolete" check box by following the twisty maze attached to the "Details" link on the "Attachments" box. ---- Anyways, review comments. This looks fine, on the whole. OK to land providing the apparently-bogus faddp implementation issue is resolved. Most of the code in this patch is, I assume, boilerplate test-support functions taken from other tests in none/tests/arm64. + /* Temporary clause to test hwcaps functionality. */ + if (bitU == 0 && sz <= X00 && opcode == BITS5(0,1,1,0,1)) { + /* -------- 0,00,01101 ADDP h_2h -------- */ + if ((archinfo->hwcaps & VEX_HWCAPS_ARM64_FP16) == 0) + return False; + DIP("faddp h%u, v%u.2h\n", dd, nn); + vex_printf("Unimplemented v8.2 instruction scalar FADDP: 0x%08x\n", insn); + return True; + } Should this be present? It certainly looks like it shouldn't be; if it accepts the instruction then it gives no implementation for it, which is definitely bogus. +static Double randDouble ( void ) +{ + ensure_special_values_initted(); + UChar c = randUChar(); + if (c >= 128) { + // return a normal number most of the time. "most" is somewhat misleading; "half" might be more accurate. Ditto in randFloat. > tests/arm64_features.c Just as a general sanity check .. is the way that the capabilities are established (by looking at auxv HWCAPS) consistent with the outcome of the discussion in bug 414268 ? I'm not looking for any change here. Instead I just want to check that there's an overall top-level coherent story about how capabilities in V are done for AArch64.
Created attachment 134154 [details] Adds arm64 feature detection for regression tests and v8.2 faddp Patch update after first set of review comments (on 2020-12-09 10:10:23 UTC).
> Most of the code in this patch is, I assume, boilerplate test-support > functions taken from other tests in none/tests/arm64. Yes but it includes additional code for testing half-precision floating-point instructions. See comment at Float16 definition. > Should this be present? It certainly looks like it shouldn't be; if it > accepts the instruction then it gives no implementation for it, which is > definitely bogus. Added implementation and tests for scalar version of half-precision FADDP. The hope is that this can be the template for future patches supporting v8.2 instructions. > "most" is somewhat misleading; "half" might be more accurate. Ditto in randFloat. Done. > Just as a general sanity check .. is the way that the capabilities are > established (by looking at auxv HWCAPS) consistent with the outcome of the > discussion in bug 414268 ? Yes.
Landed, 3b1710d38cf19619242c9113a2dbe291e914a8c2. Thank you for the work on this!
tests/arm64_features is built for each platform, but sys/auxv.h is not available everywhere and it will lead to build failure such as: arm64_features.c:5:22: fatal error: sys/auxv.h: No such file or directory compilation terminated. make[3]: *** [arm64_features.o] Error 1 Can we make this #include conditional? We can check for existence of getauxval(), or we may simply exclude it for non-arm builds with something like diff --git a/tests/arm64_features.c b/tests/arm64_features.c index 916a4e2..2c5670e 100644 --- a/tests/arm64_features.c +++ b/tests/arm64_features.c @@ -1,7 +1,7 @@ #include <stdio.h> #include <stdlib.h> #include <string.h> -#if !defined(__APPLE__) +#if !defined(__APPLE__) && defined(VGA_arm64) #include <sys/auxv.h> #endif