[RFC PATCH 0/3] tests/tcg/ppc64le: fix the build of TCG tests with Clang

[RFC PATCH 0/3] tests/tcg/ppc64le: fix the build of TCG tests with Clang

[matheus.ferst]

From: Matheus Ferst <matheus.ferst@eldorado.org.br>

Based-on: https://lists.gnu.org/archive/html/qemu-devel/2022-01/msg06506.html

As the configuration scripts used -mbig and -mlittle, building PPC tests
with Clang was silently skipped. With the patch to fix these options[1],
"make check-tcg" fails because of build and runtime errors. This patch
series tries to fix some of these problems.

The first patch fixes "tests/tcg/ppc64le/mtfsf.c" by removing the
GCC-only builtins used to emit mtfsf and mffs. We can emit these insns
with inline asm instead.

The second patch addresses differences in the output of float_madds.c.
The __builtin_fmaf used in this test emits fmadds with GCC and xsmaddasp
with LLVM. The first insn had rounding errors fixed in
d04ca895dc7f ("target/ppc: Add helpers for fmadds et al"), we apply
a similar fix to xsmaddasp.

Then we have the build errors of tests/tcg/ppc64le/bcdsub.c. According
to GCC docs[2], the '-mpower8-vector' flag provides some bcdsub
builtins, so it'd be reasonable to assume that the rest of the toolchain
knows about the insn if the compiler accepts this flag. Clang supports
this flag since version 3.6[3], but the insn and builtins were only
added in LLVM 14[4]. I couldn't find a good solution. Should we write a
test to check for this insn at configuration time? Should we detect the
compiler at build time and emit the insns with ".long" and fixed
registers?

Even building with Clang 14, the test will fail in runtime because
LLVM doesn't like "__int128" in inline asm. No error or warning is
emitted, but the generated code only loads one doubleword of the VSR.
The third patch of this series avoids this issue by using a vector
type for VSR values.

Finally, it seems that the insns tested by
tests/tcg/ppc64le/byte_reverse.c are not yet supported by LLVM. Since
the configuration script uses '-mpower10' to check for POWER10 support
and Clang doesn't support this flag, "make check-tcg" doesn't fail. We
should probably change this check in the future, but since LLVM support
of POWER10 seems incomplete, I guess we can leave it for now.

[1] https://lists.gnu.org/archive/html/qemu-devel/2022-01/msg06506.html
[2] https://gcc.gnu.org/onlinedocs/gcc-8.3.0/gcc/PowerPC-AltiVec_002fVSX-Built-in-Functions.html
[3] https://github.com/llvm/llvm-project/commit/59eb767e11d4ffefb5f55409524e5c8416b2b0db
[4] https://github.com/llvm/llvm-project/commit/c933c2eb334660c131f4afc9d194fafb0cec0423

Matheus Ferst (3):
  tests/tcg/ppc64le: use inline asm instead of __builtin_mtfsf
  target/ppc: change xs[n]madd[am]sp to use float64r32_muladd
  tests/tcg/ppc64le: Use vector types instead of __int128

 target/ppc/fpu_helper.c    | 54 ++++++++--------------
 tests/tcg/ppc64le/bcdsub.c | 92 +++++++++++++++++++++-----------------
 tests/tcg/ppc64le/mtfsf.c  | 19 ++++----
 3 files changed, 80 insertions(+), 85 deletions(-)

-- 
2.25.1

[RFC PATCH 1/3] tests/tcg/ppc64le: use inline asm instead of __builtin_mtfsf

[matheus.ferst]

From: Matheus Ferst <matheus.ferst@eldorado.org.br>

LLVM/Clang does not support __builtin_mtfsf.

Signed-off-by: Matheus Ferst <matheus.ferst@eldorado.org.br>
---
 tests/tcg/ppc64le/mtfsf.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/tests/tcg/ppc64le/mtfsf.c b/tests/tcg/ppc64le/mtfsf.c
index b3d31f3637..bed5b1afa4 100644
--- a/tests/tcg/ppc64le/mtfsf.c
+++ b/tests/tcg/ppc64le/mtfsf.c
@@ -1,8 +1,12 @@
 #include <stdlib.h>
+#include <stdint.h>
 #include <assert.h>
 #include <signal.h>
 #include <sys/prctl.h>
 
+#define MTFSF(FLM, FRB) asm volatile ("mtfsf %0, %1" :: "i" (FLM), "f" (FRB))
+#define MFFS(FRT) asm("mffs %0" : "=f" (FRT))
+
 #define FPSCR_VE     7  /* Floating-point invalid operation exception enable */
 #define FPSCR_VXSOFT 10 /* Floating-point invalid operation exception (soft) */
 #define FPSCR_FI     17 /* Floating-point fraction inexact                   */
@@ -21,10 +25,7 @@ void sigfpe_handler(int sig, siginfo_t *si, void *ucontext)
 
 int main(void)
 {
-    union {
-        double d;
-        long long ll;
-    } fpscr;
+    uint64_t fpscr;
 
     struct sigaction sa = {
         .sa_sigaction = sigfpe_handler,
@@ -40,10 +41,9 @@ int main(void)
     prctl(PR_SET_FPEXC, PR_FP_EXC_PRECISE);
 
     /* First test if the FI bit is being set correctly */
-    fpscr.ll = FP_FI;
-    __builtin_mtfsf(0b11111111, fpscr.d);
-    fpscr.d = __builtin_mffs();
-    assert((fpscr.ll & FP_FI) != 0);
+    MTFSF(0b11111111, FP_FI);
+    MFFS(fpscr);
+    assert((fpscr & FP_FI) != 0);
 
     /* Then test if the deferred exception is being called correctly */
     sigaction(SIGFPE, &sa, NULL);
@@ -54,8 +54,7 @@ int main(void)
      * But if a different exception is chosen si_code check should
      * change accordingly.
      */
-    fpscr.ll = FP_VE | FP_VXSOFT;
-    __builtin_mtfsf(0b11111111, fpscr.d);
+    MTFSF(0b11111111, FP_VE | FP_VXSOFT);
 
     return 1;
 }
-- 
2.25.1

[RFC PATCH 2/3] target/ppc: change xs[n]madd[am]sp to use float64r32_muladd

[matheus.ferst]

From: Matheus Ferst <matheus.ferst@eldorado.org.br>

Change VSX Scalar Multiply-Add/Subtract Type-A/M Single Precision
helpers to use float64r32_muladd. This method should correctly handle
all rounding modes, so the workaround for float_round_nearest_even can
be dropped.

Signed-off-by: Matheus Ferst <matheus.ferst@eldorado.org.br>
---
 target/ppc/fpu_helper.c | 54 +++++++++++++++--------------------------
 1 file changed, 19 insertions(+), 35 deletions(-)

diff --git a/target/ppc/fpu_helper.c b/target/ppc/fpu_helper.c
index e5c29b53b8..bb2a71d195 100644
--- a/target/ppc/fpu_helper.c
+++ b/target/ppc/fpu_helper.c
@@ -2157,7 +2157,7 @@ VSX_TSQRT(xvtsqrtsp, 4, float32, VsrW(i), -126, 23)
  *           various forms (madd, msub, nmadd, nmsub)
  *   sfprf - set FPRF
  */
-#define VSX_MADD(op, nels, tp, fld, maddflgs, sfprf, r2sp)                    \
+#define VSX_MADD(op, nels, tp, fld, maddflgs, sfprf)                          \
 void helper_##op(CPUPPCState *env, ppc_vsr_t *xt,                             \
                  ppc_vsr_t *xa, ppc_vsr_t *b, ppc_vsr_t *c)                   \
 {                                                                             \
@@ -2169,20 +2169,8 @@ void helper_##op(CPUPPCState *env, ppc_vsr_t *xt,                             \
     for (i = 0; i < nels; i++) {                                              \
         float_status tstat = env->fp_status;                                  \
         set_float_exception_flags(0, &tstat);                                 \
-        if (r2sp && (tstat.float_rounding_mode == float_round_nearest_even)) {\
-            /*                                                                \
-             * Avoid double rounding errors by rounding the intermediate      \
-             * result to odd.                                                 \
-             */                                                               \
-            set_float_rounding_mode(float_round_to_zero, &tstat);             \
-            t.fld = tp##_muladd(xa->fld, b->fld, c->fld,                      \
-                                maddflgs, &tstat);                            \
-            t.fld |= (get_float_exception_flags(&tstat) &                     \
-                      float_flag_inexact) != 0;                               \
-        } else {                                                              \
-            t.fld = tp##_muladd(xa->fld, b->fld, c->fld,                      \
-                                maddflgs, &tstat);                            \
-        }                                                                     \
+        t.fld = tp##_muladd(xa->fld, b->fld, c->fld,                          \
+                            maddflgs, &tstat);                                \
         env->fp_status.float_exception_flags |= tstat.float_exception_flags;  \
                                                                               \
         if (unlikely(tstat.float_exception_flags & float_flag_invalid)) {     \
@@ -2190,10 +2178,6 @@ void helper_##op(CPUPPCState *env, ppc_vsr_t *xt,                             \
                                   sfprf, GETPC());                            \
         }                                                                     \
                                                                               \
-        if (r2sp) {                                                           \
-            t.fld = do_frsp(env, t.fld, GETPC());                             \
-        }                                                                     \
-                                                                              \
         if (sfprf) {                                                          \
             helper_compute_fprf_float64(env, t.fld);                          \
         }                                                                     \
@@ -2202,24 +2186,24 @@ void helper_##op(CPUPPCState *env, ppc_vsr_t *xt,                             \
     do_float_check_status(env, GETPC());                                      \
 }
 
-VSX_MADD(xsmadddp, 1, float64, VsrD(0), MADD_FLGS, 1, 0)
-VSX_MADD(xsmsubdp, 1, float64, VsrD(0), MSUB_FLGS, 1, 0)
-VSX_MADD(xsnmadddp, 1, float64, VsrD(0), NMADD_FLGS, 1, 0)
-VSX_MADD(xsnmsubdp, 1, float64, VsrD(0), NMSUB_FLGS, 1, 0)
-VSX_MADD(xsmaddsp, 1, float64, VsrD(0), MADD_FLGS, 1, 1)
-VSX_MADD(xsmsubsp, 1, float64, VsrD(0), MSUB_FLGS, 1, 1)
-VSX_MADD(xsnmaddsp, 1, float64, VsrD(0), NMADD_FLGS, 1, 1)
-VSX_MADD(xsnmsubsp, 1, float64, VsrD(0), NMSUB_FLGS, 1, 1)
+VSX_MADD(xsmadddp, 1, float64, VsrD(0), MADD_FLGS, 1)
+VSX_MADD(xsmsubdp, 1, float64, VsrD(0), MSUB_FLGS, 1)
+VSX_MADD(xsnmadddp, 1, float64, VsrD(0), NMADD_FLGS, 1)
+VSX_MADD(xsnmsubdp, 1, float64, VsrD(0), NMSUB_FLGS, 1)
+VSX_MADD(xsmaddsp, 1, float64r32, VsrD(0), MADD_FLGS, 1)
+VSX_MADD(xsmsubsp, 1, float64r32, VsrD(0), MSUB_FLGS, 1)
+VSX_MADD(xsnmaddsp, 1, float64r32, VsrD(0), NMADD_FLGS, 1)
+VSX_MADD(xsnmsubsp, 1, float64r32, VsrD(0), NMSUB_FLGS, 1)
 
-VSX_MADD(xvmadddp, 2, float64, VsrD(i), MADD_FLGS, 0, 0)
-VSX_MADD(xvmsubdp, 2, float64, VsrD(i), MSUB_FLGS, 0, 0)
-VSX_MADD(xvnmadddp, 2, float64, VsrD(i), NMADD_FLGS, 0, 0)
-VSX_MADD(xvnmsubdp, 2, float64, VsrD(i), NMSUB_FLGS, 0, 0)
+VSX_MADD(xvmadddp, 2, float64, VsrD(i), MADD_FLGS, 0)
+VSX_MADD(xvmsubdp, 2, float64, VsrD(i), MSUB_FLGS, 0)
+VSX_MADD(xvnmadddp, 2, float64, VsrD(i), NMADD_FLGS, 0)
+VSX_MADD(xvnmsubdp, 2, float64, VsrD(i), NMSUB_FLGS, 0)
 
-VSX_MADD(xvmaddsp, 4, float32, VsrW(i), MADD_FLGS, 0, 0)
-VSX_MADD(xvmsubsp, 4, float32, VsrW(i), MSUB_FLGS, 0, 0)
-VSX_MADD(xvnmaddsp, 4, float32, VsrW(i), NMADD_FLGS, 0, 0)
-VSX_MADD(xvnmsubsp, 4, float32, VsrW(i), NMSUB_FLGS, 0, 0)
+VSX_MADD(xvmaddsp, 4, float32, VsrW(i), MADD_FLGS, 0)
+VSX_MADD(xvmsubsp, 4, float32, VsrW(i), MSUB_FLGS, 0)
+VSX_MADD(xvnmaddsp, 4, float32, VsrW(i), NMADD_FLGS, 0)
+VSX_MADD(xvnmsubsp, 4, float32, VsrW(i), NMSUB_FLGS, 0)
 
 /*
  * VSX_SCALAR_CMP_DP - VSX scalar floating point compare double precision
-- 
2.25.1

[RFC PATCH 3/3] tests/tcg/ppc64le: Use vector types instead of __int128

[matheus.ferst]

From: Matheus Ferst <matheus.ferst@eldorado.org.br>

LLVM/Clang doesn't like inline asm with __int128, use a vector type
instead.

Signed-off-by: Matheus Ferst <matheus.ferst@eldorado.org.br>
---
Alternatively, we could pass VSR values in GPR pairs, as we did in
tests/tcg/ppc64le/non_signalling_xscv.c
---
 tests/tcg/ppc64le/bcdsub.c | 92 +++++++++++++++++++++-----------------
 1 file changed, 52 insertions(+), 40 deletions(-)

diff --git a/tests/tcg/ppc64le/bcdsub.c b/tests/tcg/ppc64le/bcdsub.c
index 8c188cae6d..17403daf22 100644
--- a/tests/tcg/ppc64le/bcdsub.c
+++ b/tests/tcg/ppc64le/bcdsub.c
@@ -1,6 +1,7 @@
 #include <assert.h>
 #include <unistd.h>
 #include <signal.h>
+#include <altivec.h>
 
 #define CRF_LT  (1 << 3)
 #define CRF_GT  (1 << 2)
@@ -8,6 +9,16 @@
 #define CRF_SO  (1 << 0)
 #define UNDEF   0
 
+#ifdef __LITTLE_ENDIAN__
+#define HIGH(T) (T)[1]
+#define LOW(T) (T)[0]
+#define U128(H, L) (vector unsigned long long) {L, H}
+#else
+#define HIGH(T) (T)[0]
+#define LOW(T) (T)[1]
+#define U128(H, L) (vector unsigned long long) {H, L}
+#endif
+
 #define BCDSUB(vra, vrb, ps)                    \
     asm ("bcdsub. %1,%2,%3,%4;"                 \
          "mfocrf %0,0b10;"                      \
@@ -15,17 +26,18 @@
          : "v" (vra), "v" (vrb), "i" (ps)       \
          : );
 
-#define TEST(vra, vrb, ps, exp_res, exp_cr6)    \
-    do {                                        \
-        __int128 vrt = 0;                       \
-        int cr = 0;                             \
-        BCDSUB(vra, vrb, ps);                   \
-        if (exp_res)                            \
-            assert(vrt == exp_res);             \
-        assert((cr >> 4) == exp_cr6);           \
+#define TEST(vra, vrb, ps, exp_res_h, exp_res_l, exp_cr6)   \
+    do {                                                    \
+        vector unsigned long long vrt = U128(0, 0);         \
+        int cr = 0;                                         \
+        BCDSUB(vra, vrb, ps);                               \
+        if (exp_res_h || exp_res_l) {                       \
+            assert(HIGH(vrt) == exp_res_h);                 \
+            assert(LOW(vrt) == exp_res_l);                  \
+        }                                                   \
+        assert((cr >> 4) == exp_cr6);                       \
     } while (0)
 
-
 /*
  * Unbounded result is equal to zero:
  *   sign = (PS) ? 0b1111 : 0b1100
@@ -33,13 +45,13 @@
  */
 void test_bcdsub_eq(void)
 {
-    __int128 a, b;
+    vector unsigned long long a, b;
 
     /* maximum positive BCD value */
-    a = b = (((__int128) 0x9999999999999999) << 64 | 0x999999999999999c);
+    a = b = U128(0x9999999999999999, 0x999999999999999c);
 
-    TEST(a, b, 0, 0xc, CRF_EQ);
-    TEST(a, b, 1, 0xf, CRF_EQ);
+    TEST(a, b, 0, 0x0, 0xc, CRF_EQ);
+    TEST(a, b, 1, 0x0, 0xf, CRF_EQ);
 }
 
 /*
@@ -49,21 +61,21 @@ void test_bcdsub_eq(void)
  */
 void test_bcdsub_gt(void)
 {
-    __int128 a, b, c;
+    vector unsigned long long a, b, c;
 
     /* maximum positive BCD value */
-    a = (((__int128) 0x9999999999999999) << 64 | 0x999999999999999c);
+    a = U128(0x9999999999999999, 0x999999999999999c);
 
     /* negative one BCD value */
-    b = (__int128) 0x1d;
+    b = U128(0x0, 0x1d);
 
-    TEST(a, b, 0, 0xc, (CRF_GT | CRF_SO));
-    TEST(a, b, 1, 0xf, (CRF_GT | CRF_SO));
+    TEST(a, b, 0, 0x0, 0xc, (CRF_GT | CRF_SO));
+    TEST(a, b, 1, 0x0, 0xf, (CRF_GT | CRF_SO));
 
-    c = (((__int128) 0x9999999999999999) << 64 | 0x999999999999998c);
+    c = U128(0x9999999999999999, 0x999999999999998c);
 
-    TEST(c, b, 0, a, CRF_GT);
-    TEST(c, b, 1, (a | 0x3), CRF_GT);
+    TEST(c, b, 0, HIGH(a), LOW(a), CRF_GT);
+    TEST(c, b, 1, HIGH(a), (LOW(a) | 0x3), CRF_GT);
 }
 
 /*
@@ -73,45 +85,45 @@ void test_bcdsub_gt(void)
  */
 void test_bcdsub_lt(void)
 {
-    __int128 a, b;
+    vector unsigned long long a, b;
 
     /* positive zero BCD value */
-    a = (__int128) 0xc;
+    a = U128(0x0, 0xc);
 
     /* positive one BCD value */
-    b = (__int128) 0x1c;
+    b = U128(0x0, 0x1c);
 
-    TEST(a, b, 0, 0x1d, CRF_LT);
-    TEST(a, b, 1, 0x1d, CRF_LT);
+    TEST(a, b, 0, 0x0, 0x1d, CRF_LT);
+    TEST(a, b, 1, 0x0, 0x1d, CRF_LT);
 
     /* maximum negative BCD value */
-    a = (((__int128) 0x9999999999999999) << 64 | 0x999999999999999d);
+    a = U128(0x9999999999999999, 0x999999999999999d);
 
     /* positive one BCD value */
-    b = (__int128) 0x1c;
+    b = U128(0x0, 0x1c);
 
-    TEST(a, b, 0, 0xd, (CRF_LT | CRF_SO));
-    TEST(a, b, 1, 0xd, (CRF_LT | CRF_SO));
+    TEST(a, b, 0, 0x0, 0xd, (CRF_LT | CRF_SO));
+    TEST(a, b, 1, 0x0, 0xd, (CRF_LT | CRF_SO));
 }
 
 void test_bcdsub_invalid(void)
 {
-    __int128 a, b;
+    vector unsigned long long a, b;
 
     /* positive one BCD value */
-    a = (__int128) 0x1c;
-    b = 0xf00;
+    a = U128(0x0, 0x1c);
+    b = U128(0x0, 0xf00);
 
-    TEST(a, b, 0, UNDEF, CRF_SO);
-    TEST(a, b, 1, UNDEF, CRF_SO);
+    TEST(a, b, 0, UNDEF, UNDEF, CRF_SO);
+    TEST(a, b, 1, UNDEF, UNDEF, CRF_SO);
 
-    TEST(b, a, 0, UNDEF, CRF_SO);
-    TEST(b, a, 1, UNDEF, CRF_SO);
+    TEST(b, a, 0, UNDEF, UNDEF, CRF_SO);
+    TEST(b, a, 1, UNDEF, UNDEF, CRF_SO);
 
-    a = 0xbad;
+    a = U128(0x0, 0xbad);
 
-    TEST(a, b, 0, UNDEF, CRF_SO);
-    TEST(a, b, 1, UNDEF, CRF_SO);
+    TEST(a, b, 0, UNDEF, UNDEF, CRF_SO);
+    TEST(a, b, 1, UNDEF, UNDEF, CRF_SO);
 }
 
 int main(void)
-- 
2.25.1

Re: [RFC PATCH 0/3] tests/tcg/ppc64le: fix the build of TCG tests with Clang

[Cédric Le Goater]

Hello Matheus,

[ Adding Miroslav ]

On 2/8/22 21:31, matheus.ferst@eldorado.org.br wrote:
> From: Matheus Ferst <matheus.ferst@eldorado.org.br>
> 
> Based-on: https://lists.gnu.org/archive/html/qemu-devel/2022-01/msg06506.html
> 
> As the configuration scripts used -mbig and -mlittle, building PPC tests
> with Clang was silently skipped. With the patch to fix these options[1],
> "make check-tcg" fails because of build and runtime errors. This patch
> series tries to fix some of these problems.
> 
> The first patch fixes "tests/tcg/ppc64le/mtfsf.c" by removing the
> GCC-only builtins used to emit mtfsf and mffs. We can emit these insns
> with inline asm instead.
> 
> The second patch addresses differences in the output of float_madds.c.
> The __builtin_fmaf used in this test emits fmadds with GCC and xsmaddasp
> with LLVM. The first insn had rounding errors fixed in
> d04ca895dc7f ("target/ppc: Add helpers for fmadds et al"), we apply
> a similar fix to xsmaddasp.
> 
> Then we have the build errors of tests/tcg/ppc64le/bcdsub.c. According
> to GCC docs[2], the '-mpower8-vector' flag provides some bcdsub
> builtins, so it'd be reasonable to assume that the rest of the toolchain
> knows about the insn if the compiler accepts this flag. Clang supports
> this flag since version 3.6[3], but the insn and builtins were only
> added in LLVM 14[4]. I couldn't find a good solution. Should we write a
> test to check for this insn at configuration time? Should we detect the
> compiler at build time and emit the insns with ".long" and fixed
> registers?
> 
> Even building with Clang 14, the test will fail in runtime because
> LLVM doesn't like "__int128" in inline asm. No error or warning is
> emitted, but the generated code only loads one doubleword of the VSR.
> The third patch of this series avoids this issue by using a vector
> type for VSR values.
> 
> Finally, it seems that the insns tested by
> tests/tcg/ppc64le/byte_reverse.c are not yet supported by LLVM. Since
> the configuration script uses '-mpower10' to check for POWER10 support
> and Clang doesn't support this flag, "make check-tcg" doesn't fail. We
> should probably change this check in the future, but since LLVM support
> of POWER10 seems incomplete, I guess we can leave it for now.

gitlab didn't spot any issues with the 4 patches applied. Should we merge
all patches :

   Use long endian options for ppc64
   tests/tcg/ppc64le: Use vector types instead of __int128
   target/ppc: change xs[n]madd[am]sp to use float64r32_muladd
   tests/tcg/ppc64le: use inline asm instead of __builtin_mtfsf

and see how we can address the LLVM support for P10 later ?

Thanks,

C.
  
> [1] https://lists.gnu.org/archive/html/qemu-devel/2022-01/msg06506.html
> [2] https://gcc.gnu.org/onlinedocs/gcc-8.3.0/gcc/PowerPC-AltiVec_002fVSX-Built-in-Functions.html
> [3] https://github.com/llvm/llvm-project/commit/59eb767e11d4ffefb5f55409524e5c8416b2b0db
> [4] https://github.com/llvm/llvm-project/commit/c933c2eb334660c131f4afc9d194fafb0cec0423
> 
> Matheus Ferst (3):
>    tests/tcg/ppc64le: use inline asm instead of __builtin_mtfsf
>    target/ppc: change xs[n]madd[am]sp to use float64r32_muladd
>    tests/tcg/ppc64le: Use vector types instead of __int128
> 
>   target/ppc/fpu_helper.c    | 54 ++++++++--------------
>   tests/tcg/ppc64le/bcdsub.c | 92 +++++++++++++++++++++-----------------
>   tests/tcg/ppc64le/mtfsf.c  | 19 ++++----
>   3 files changed, 80 insertions(+), 85 deletions(-)
> 

Re: [RFC PATCH 1/3] tests/tcg/ppc64le: use inline asm instead of __builtin_mtfsf

[Alex Bennée]

matheus.ferst@eldorado.org.br writes:

> From: Matheus Ferst <matheus.ferst@eldorado.org.br>
>
> LLVM/Clang does not support __builtin_mtfsf.
>
> Signed-off-by: Matheus Ferst <matheus.ferst@eldorado.org.br>

Acked-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée

Re: [RFC PATCH 2/3] target/ppc: change xs[n]madd[am]sp to use float64r32_muladd

[Richard Henderson]

On 2/9/22 07:31, matheus.ferst@eldorado.org.br wrote:
> From: Matheus Ferst<matheus.ferst@eldorado.org.br>
> 
> Change VSX Scalar Multiply-Add/Subtract Type-A/M Single Precision
> helpers to use float64r32_muladd. This method should correctly handle
> all rounding modes, so the workaround for float_round_nearest_even can
> be dropped.
> 
> Signed-off-by: Matheus Ferst<matheus.ferst@eldorado.org.br>
> ---
>   target/ppc/fpu_helper.c | 54 +++++++++++++++--------------------------
>   1 file changed, 19 insertions(+), 35 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [RFC PATCH 0/3] tests/tcg/ppc64le: fix the build of TCG tests with Clang

[Richard Henderson]

On 2/9/22 07:31, matheus.ferst@eldorado.org.br wrote:
> The second patch addresses differences in the output of float_madds.c.
> The __builtin_fmaf used in this test emits fmadds with GCC and xsmaddasp
> with LLVM. The first insn had rounding errors fixed in
> d04ca895dc7f ("target/ppc: Add helpers for fmadds et al"), we apply
> a similar fix to xsmaddasp.

Thanks for this.  I missed those before.

There are a number of other missed vector cases, which you may have seen.
Basically, anything with "r2sp" needs updating.


r~

Re: [RFC PATCH 0/3] tests/tcg/ppc64le: fix the build of TCG tests with Clang

[Matheus K. Ferst]

On 09/02/2022 09:31, Cédric Le Goater wrote:
> Hello Matheus,
> 
> [ Adding Miroslav ]
> 
> On 2/8/22 21:31, matheus.ferst@eldorado.org.br wrote:
>> From: Matheus Ferst <matheus.ferst@eldorado.org.br>
>>
>> Based-on: 
>> https://lists.gnu.org/archive/html/qemu-devel/2022-01/msg06506.html
>>
>> As the configuration scripts used -mbig and -mlittle, building PPC tests
>> with Clang was silently skipped. With the patch to fix these options[1],
>> "make check-tcg" fails because of build and runtime errors. This patch
>> series tries to fix some of these problems.
>>
>> The first patch fixes "tests/tcg/ppc64le/mtfsf.c" by removing the
>> GCC-only builtins used to emit mtfsf and mffs. We can emit these insns
>> with inline asm instead.
>>
>> The second patch addresses differences in the output of float_madds.c.
>> The __builtin_fmaf used in this test emits fmadds with GCC and xsmaddasp
>> with LLVM. The first insn had rounding errors fixed in
>> d04ca895dc7f ("target/ppc: Add helpers for fmadds et al"), we apply
>> a similar fix to xsmaddasp.
>>
>> Then we have the build errors of tests/tcg/ppc64le/bcdsub.c. According
>> to GCC docs[2], the '-mpower8-vector' flag provides some bcdsub
>> builtins, so it'd be reasonable to assume that the rest of the toolchain
>> knows about the insn if the compiler accepts this flag. Clang supports
>> this flag since version 3.6[3], but the insn and builtins were only
>> added in LLVM 14[4]. I couldn't find a good solution. Should we write a
>> test to check for this insn at configuration time? Should we detect the
>> compiler at build time and emit the insns with ".long" and fixed
>> registers?
>>
>> Even building with Clang 14, the test will fail in runtime because
>> LLVM doesn't like "__int128" in inline asm. No error or warning is
>> emitted, but the generated code only loads one doubleword of the VSR.
>> The third patch of this series avoids this issue by using a vector
>> type for VSR values.
>>
>> Finally, it seems that the insns tested by
>> tests/tcg/ppc64le/byte_reverse.c are not yet supported by LLVM. Since
>> the configuration script uses '-mpower10' to check for POWER10 support
>> and Clang doesn't support this flag, "make check-tcg" doesn't fail. We
>> should probably change this check in the future, but since LLVM support
>> of POWER10 seems incomplete, I guess we can leave it for now.
> 
> gitlab didn't spot any issues with the 4 patches applied.


AFAICT, CI wouldn't run into this kind of problem because we don't have 
PPC runners, and the cross-compiler containers use GCC.

> Should we merge all patches :
> 
>    Use long endian options for ppc64
>    tests/tcg/ppc64le: Use vector types instead of __int128
>    target/ppc: change xs[n]madd[am]sp to use float64r32_muladd
>    tests/tcg/ppc64le: use inline asm instead of __builtin_mtfsf
> 
> and see how we can address the LLVM support for P10 later ?
> 

The problems with bcdsub.c are not resolved for Clang < 14, but I guess 
it's ok to merge anyway.

Thanks,
Matheus K. Ferst
Instituto de Pesquisas ELDORADO <http://www.eldorado.org.br/>
Analista de Software
Aviso Legal - Disclaimer <https://www.eldorado.org.br/disclaimer.html>

Re: [RFC PATCH 3/3] tests/tcg/ppc64le: Use vector types instead of __int128

[Cédric Le Goater]

On 2/8/22 21:31, matheus.ferst@eldorado.org.br wrote:
> From: Matheus Ferst <matheus.ferst@eldorado.org.br>
> 
> LLVM/Clang doesn't like inline asm with __int128, use a vector type
> instead.
> 
> Signed-off-by: Matheus Ferst <matheus.ferst@eldorado.org.br>
> ---
> Alternatively, we could pass VSR values in GPR pairs, as we did in
> tests/tcg/ppc64le/non_signalling_xscv.c
> ---
>   tests/tcg/ppc64le/bcdsub.c | 92 +++++++++++++++++++++-----------------
>   1 file changed, 52 insertions(+), 40 deletions(-)
> 
> diff --git a/tests/tcg/ppc64le/bcdsub.c b/tests/tcg/ppc64le/bcdsub.c
> index 8c188cae6d..17403daf22 100644
> --- a/tests/tcg/ppc64le/bcdsub.c
> +++ b/tests/tcg/ppc64le/bcdsub.c
> @@ -1,6 +1,7 @@
>   #include <assert.h>
>   #include <unistd.h>
>   #include <signal.h>
> +#include <altivec.h>
>   
>   #define CRF_LT  (1 << 3)
>   #define CRF_GT  (1 << 2)
> @@ -8,6 +9,16 @@
>   #define CRF_SO  (1 << 0)
>   #define UNDEF   0
>   
> +#ifdef __LITTLE_ENDIAN__

Shouldn't we be using :

#if BYTE_ORDER == LITTLE_ENDIAN

instead ?

Thanks,

C.

> +#define HIGH(T) (T)[1]
> +#define LOW(T) (T)[0]
> +#define U128(H, L) (vector unsigned long long) {L, H}
> +#else
> +#define HIGH(T) (T)[0]
> +#define LOW(T) (T)[1]
> +#define U128(H, L) (vector unsigned long long) {H, L}
> +#endif
> +
>   #define BCDSUB(vra, vrb, ps)                    \
>       asm ("bcdsub. %1,%2,%3,%4;"                 \
>            "mfocrf %0,0b10;"                      \
> @@ -15,17 +26,18 @@
>            : "v" (vra), "v" (vrb), "i" (ps)       \
>            : );
>   
> -#define TEST(vra, vrb, ps, exp_res, exp_cr6)    \
> -    do {                                        \
> -        __int128 vrt = 0;                       \
> -        int cr = 0;                             \
> -        BCDSUB(vra, vrb, ps);                   \
> -        if (exp_res)                            \
> -            assert(vrt == exp_res);             \
> -        assert((cr >> 4) == exp_cr6);           \
> +#define TEST(vra, vrb, ps, exp_res_h, exp_res_l, exp_cr6)   \
> +    do {                                                    \
> +        vector unsigned long long vrt = U128(0, 0);         \
> +        int cr = 0;                                         \
> +        BCDSUB(vra, vrb, ps);                               \
> +        if (exp_res_h || exp_res_l) {                       \
> +            assert(HIGH(vrt) == exp_res_h);                 \
> +            assert(LOW(vrt) == exp_res_l);                  \
> +        }                                                   \
> +        assert((cr >> 4) == exp_cr6);                       \
>       } while (0)
>   
> -
>   /*
>    * Unbounded result is equal to zero:
>    *   sign = (PS) ? 0b1111 : 0b1100
> @@ -33,13 +45,13 @@
>    */
>   void test_bcdsub_eq(void)
>   {
> -    __int128 a, b;
> +    vector unsigned long long a, b;
>   
>       /* maximum positive BCD value */
> -    a = b = (((__int128) 0x9999999999999999) << 64 | 0x999999999999999c);
> +    a = b = U128(0x9999999999999999, 0x999999999999999c);
>   
> -    TEST(a, b, 0, 0xc, CRF_EQ);
> -    TEST(a, b, 1, 0xf, CRF_EQ);
> +    TEST(a, b, 0, 0x0, 0xc, CRF_EQ);
> +    TEST(a, b, 1, 0x0, 0xf, CRF_EQ);
>   }
>   
>   /*
> @@ -49,21 +61,21 @@ void test_bcdsub_eq(void)
>    */
>   void test_bcdsub_gt(void)
>   {
> -    __int128 a, b, c;
> +    vector unsigned long long a, b, c;
>   
>       /* maximum positive BCD value */
> -    a = (((__int128) 0x9999999999999999) << 64 | 0x999999999999999c);
> +    a = U128(0x9999999999999999, 0x999999999999999c);
>   
>       /* negative one BCD value */
> -    b = (__int128) 0x1d;
> +    b = U128(0x0, 0x1d);
>   
> -    TEST(a, b, 0, 0xc, (CRF_GT | CRF_SO));
> -    TEST(a, b, 1, 0xf, (CRF_GT | CRF_SO));
> +    TEST(a, b, 0, 0x0, 0xc, (CRF_GT | CRF_SO));
> +    TEST(a, b, 1, 0x0, 0xf, (CRF_GT | CRF_SO));
>   
> -    c = (((__int128) 0x9999999999999999) << 64 | 0x999999999999998c);
> +    c = U128(0x9999999999999999, 0x999999999999998c);
>   
> -    TEST(c, b, 0, a, CRF_GT);
> -    TEST(c, b, 1, (a | 0x3), CRF_GT);
> +    TEST(c, b, 0, HIGH(a), LOW(a), CRF_GT);
> +    TEST(c, b, 1, HIGH(a), (LOW(a) | 0x3), CRF_GT);
>   }
>   
>   /*
> @@ -73,45 +85,45 @@ void test_bcdsub_gt(void)
>    */
>   void test_bcdsub_lt(void)
>   {
> -    __int128 a, b;
> +    vector unsigned long long a, b;
>   
>       /* positive zero BCD value */
> -    a = (__int128) 0xc;
> +    a = U128(0x0, 0xc);
>   
>       /* positive one BCD value */
> -    b = (__int128) 0x1c;
> +    b = U128(0x0, 0x1c);
>   
> -    TEST(a, b, 0, 0x1d, CRF_LT);
> -    TEST(a, b, 1, 0x1d, CRF_LT);
> +    TEST(a, b, 0, 0x0, 0x1d, CRF_LT);
> +    TEST(a, b, 1, 0x0, 0x1d, CRF_LT);
>   
>       /* maximum negative BCD value */
> -    a = (((__int128) 0x9999999999999999) << 64 | 0x999999999999999d);
> +    a = U128(0x9999999999999999, 0x999999999999999d);
>   
>       /* positive one BCD value */
> -    b = (__int128) 0x1c;
> +    b = U128(0x0, 0x1c);
>   
> -    TEST(a, b, 0, 0xd, (CRF_LT | CRF_SO));
> -    TEST(a, b, 1, 0xd, (CRF_LT | CRF_SO));
> +    TEST(a, b, 0, 0x0, 0xd, (CRF_LT | CRF_SO));
> +    TEST(a, b, 1, 0x0, 0xd, (CRF_LT | CRF_SO));
>   }
>   
>   void test_bcdsub_invalid(void)
>   {
> -    __int128 a, b;
> +    vector unsigned long long a, b;
>   
>       /* positive one BCD value */
> -    a = (__int128) 0x1c;
> -    b = 0xf00;
> +    a = U128(0x0, 0x1c);
> +    b = U128(0x0, 0xf00);
>   
> -    TEST(a, b, 0, UNDEF, CRF_SO);
> -    TEST(a, b, 1, UNDEF, CRF_SO);
> +    TEST(a, b, 0, UNDEF, UNDEF, CRF_SO);
> +    TEST(a, b, 1, UNDEF, UNDEF, CRF_SO);
>   
> -    TEST(b, a, 0, UNDEF, CRF_SO);
> -    TEST(b, a, 1, UNDEF, CRF_SO);
> +    TEST(b, a, 0, UNDEF, UNDEF, CRF_SO);
> +    TEST(b, a, 1, UNDEF, UNDEF, CRF_SO);
>   
> -    a = 0xbad;
> +    a = U128(0x0, 0xbad);
>   
> -    TEST(a, b, 0, UNDEF, CRF_SO);
> -    TEST(a, b, 1, UNDEF, CRF_SO);
> +    TEST(a, b, 0, UNDEF, UNDEF, CRF_SO);
> +    TEST(a, b, 1, UNDEF, UNDEF, CRF_SO);
>   }
>   
>   int main(void)

Re: [RFC PATCH 3/3] tests/tcg/ppc64le: Use vector types instead of __int128

[Matheus K. Ferst]

On 17/02/2022 05:09, Cédric Le Goater wrote:
> On 2/8/22 21:31, matheus.ferst@eldorado.org.br wrote:
>> From: Matheus Ferst <matheus.ferst@eldorado.org.br>
>>
>> LLVM/Clang doesn't like inline asm with __int128, use a vector type
>> instead.
>>
>> Signed-off-by: Matheus Ferst <matheus.ferst@eldorado.org.br>
>> ---
>> Alternatively, we could pass VSR values in GPR pairs, as we did in
>> tests/tcg/ppc64le/non_signalling_xscv.c
>> ---
>>   tests/tcg/ppc64le/bcdsub.c | 92 +++++++++++++++++++++-----------------
>>   1 file changed, 52 insertions(+), 40 deletions(-)
>>
>> diff --git a/tests/tcg/ppc64le/bcdsub.c b/tests/tcg/ppc64le/bcdsub.c
>> index 8c188cae6d..17403daf22 100644
>> --- a/tests/tcg/ppc64le/bcdsub.c
>> +++ b/tests/tcg/ppc64le/bcdsub.c
>> @@ -1,6 +1,7 @@
>>   #include <assert.h>
>>   #include <unistd.h>
>>   #include <signal.h>
>> +#include <altivec.h>
>>
>>   #define CRF_LT  (1 << 3)
>>   #define CRF_GT  (1 << 2)
>> @@ -8,6 +9,16 @@
>>   #define CRF_SO  (1 << 0)
>>   #define UNDEF   0
>>
>> +#ifdef __LITTLE_ENDIAN__
> 
> Shouldn't we be using :
> 
> #if BYTE_ORDER == LITTLE_ENDIAN
> 
> instead ?
> 

I guess it is better, I'll send a v2.

Thanks,
Matheus K. Ferst
Instituto de Pesquisas ELDORADO <http://www.eldorado.org.br/>
Analista de Software
Aviso Legal - Disclaimer <https://www.eldorado.org.br/disclaimer.html>

Re: [RFC PATCH 3/3] tests/tcg/ppc64le: Use vector types instead of __int128

[Matheus K. Ferst]

On 17/02/2022 09:46, Matheus K. Ferst wrote:
> On 17/02/2022 05:09, Cédric Le Goater wrote:
>> On 2/8/22 21:31, matheus.ferst@eldorado.org.br wrote:
>>> From: Matheus Ferst <matheus.ferst@eldorado.org.br>
>>>
>>> LLVM/Clang doesn't like inline asm with __int128, use a vector type
>>> instead.
>>>
>>> Signed-off-by: Matheus Ferst <matheus.ferst@eldorado.org.br>
>>> ---
>>> Alternatively, we could pass VSR values in GPR pairs, as we did in
>>> tests/tcg/ppc64le/non_signalling_xscv.c
>>> ---
>>>   tests/tcg/ppc64le/bcdsub.c | 92 +++++++++++++++++++++-----------------
>>>   1 file changed, 52 insertions(+), 40 deletions(-)
>>>
>>> diff --git a/tests/tcg/ppc64le/bcdsub.c b/tests/tcg/ppc64le/bcdsub.c
>>> index 8c188cae6d..17403daf22 100644
>>> --- a/tests/tcg/ppc64le/bcdsub.c
>>> +++ b/tests/tcg/ppc64le/bcdsub.c
>>> @@ -1,6 +1,7 @@
>>>   #include <assert.h>
>>>   #include <unistd.h>
>>>   #include <signal.h>
>>> +#include <altivec.h>
>>>
>>>   #define CRF_LT  (1 << 3)
>>>   #define CRF_GT  (1 << 2)
>>> @@ -8,6 +9,16 @@
>>>   #define CRF_SO  (1 << 0)
>>>   #define UNDEF   0
>>>
>>> +#ifdef __LITTLE_ENDIAN__
>>
>> Shouldn't we be using :
>>
>> #if BYTE_ORDER == LITTLE_ENDIAN
>>
>> instead ?
>>
> 
> I guess it is better, I'll send a v2.
> 

Actually, it doesn't work for LLVM and needs endian.h for GCC[1]. This 
check is also used in sigbus and sha1 tests. The first shouldn't be a 
problem (allow_fail is zero for ppc), but sha1 gives the wrong result 
for BE:

$ ./qemu-ppc64le tests/tcg/ppc64le-linux-user/sha1
SHA1=15dd99a1991e0b3826fede3deffc1feba42278e6
$ ./qemu-ppc64 tests/tcg/ppc64-linux-user/sha1
SHA1=70f1d4d65eb47309ffacc5a28ff285ad826006da

and 'make check-tcg' succeeds anyway...

[1] https://godbolt.org/z/fYbzbbexn
-- 
Matheus K. Ferst
Instituto de Pesquisas ELDORADO <http://www.eldorado.org.br/>
Analista de Software
Aviso Legal - Disclaimer <https://www.eldorado.org.br/disclaimer.html>