Bug 413547

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