Re: [Qemu-devel] [PATCH] tcg: Fix helper function vs host abi for float16

2018-05-24 Thread Richard Henderson 
On 05/24/2018 06:21 AM, Peter Maydell wrote:
> Applied to target-arm.next, thanks. Is it worth marking this as
> cc:stable?

Probably, since we've marked most of the other f16 patches for stable.


r~

Re: [Qemu-devel] [PATCH] tcg: Fix helper function vs host abi for float16

2018-05-24 Thread Peter Maydell 
On 22 May 2018 at 18:56, Richard Henderson  wrote:
> Depending on the host abi, float16, aka uint16_t, values are
> passed and returned either zero-extended in the host register
> or with garbage at the top of the host register.
>
> The tcg code generator has so far been assuming garbage, as that
> matches the x86 abi, but this is incorrect for other host abis.
> Further, target/arm has so far been assuming zero-extended results,
> so that it may store the 16-bit value into a 32-bit slot with the
> high 16-bits already clear.
>
> Rectify both problems by mapping "f16" in the helper definition
> to uint32_t instead of (a typedef for) uint16_t.  This forces
> the host compiler to assume garbage in the upper 16 bits on input
> and to zero-extend the result on output.
>
> Signed-off-by: Richard Henderson 

Applied to target-arm.next, thanks. Is it worth marking this as
cc:stable?

thanks
-- PMM

Re: [Qemu-devel] [PATCH] tcg: Fix helper function vs host abi for float16

2018-05-24 Thread Laurent Desnogues 
On Thu, May 24, 2018 at 2:28 PM, Peter Maydell  wrote:
> On 23 May 2018 at 06:10, Laurent Desnogues  
> wrote:
>> Some AArch64 tests I had that previously failed on a x86-64 host now pass.
>>
>> Tested-by: Laurent Desnogues 
>
> Thanks for the testing.
>
>> Perhaps the two occurrences of the following comment in
>> target/arm/translate-a64.c could be removed along with this patch:
>>
>> /* write_fp_sreg is OK here because top half of tcg_rd is zero */
>
> I think this comment remains correct, doesn't it? At this point
> we've just called a helper routine which is 'f16' return. With
> this patch, that means that (by virtue of being uint16_t return
> type as far as C is concerned), it will have returned a 32 bit
> value into the TCGv_i32 whose bits [31:16] are zero, as the
> comment says. We require that because we're using
> write_fp_sreg() to update the vector register, and that function
> zeroes out bits [127:32] and assumes [31:16] from its input
> should all go into the vector register; but the instruction's
> semantics require [127:16] to be zeroed, and it's only because
> we know [31:16] are zero in the return value that we can
> get away with calling write_fp_sreg() rather than requiring a
> new write_fp_hreg().

I was reading the comment as somehow explaining the assumption of the
ABI clearing the top 16-bit of 16-bit return value.  But I agree it
still makes sense as it is.

Thanks,

Laurent

Re: [Qemu-devel] [PATCH] tcg: Fix helper function vs host abi for float16

2018-05-24 Thread Peter Maydell 
On 23 May 2018 at 06:10, Laurent Desnogues  wrote:
> Some AArch64 tests I had that previously failed on a x86-64 host now pass.
>
> Tested-by: Laurent Desnogues 

Thanks for the testing.

> Perhaps the two occurrences of the following comment in
> target/arm/translate-a64.c could be removed along with this patch:
>
> /* write_fp_sreg is OK here because top half of tcg_rd is zero */

I think this comment remains correct, doesn't it? At this point
we've just called a helper routine which is 'f16' return. With
this patch, that means that (by virtue of being uint16_t return
type as far as C is concerned), it will have returned a 32 bit
value into the TCGv_i32 whose bits [31:16] are zero, as the
comment says. We require that because we're using
write_fp_sreg() to update the vector register, and that function
zeroes out bits [127:32] and assumes [31:16] from its input
should all go into the vector register; but the instruction's
semantics require [127:16] to be zeroed, and it's only because
we know [31:16] are zero in the return value that we can
get away with calling write_fp_sreg() rather than requiring a
new write_fp_hreg().

thanks
-- PMM

Re: [Qemu-devel] [PATCH] tcg: Fix helper function vs host abi for float16

2018-05-22 Thread Laurent Desnogues 
Hello,

On Tue, May 22, 2018 at 7:56 PM, Richard Henderson
 wrote:
> Depending on the host abi, float16, aka uint16_t, values are
> passed and returned either zero-extended in the host register
> or with garbage at the top of the host register.
>
> The tcg code generator has so far been assuming garbage, as that
> matches the x86 abi, but this is incorrect for other host abis.
> Further, target/arm has so far been assuming zero-extended results,
> so that it may store the 16-bit value into a 32-bit slot with the
> high 16-bits already clear.
>
> Rectify both problems by mapping "f16" in the helper definition
> to uint32_t instead of (a typedef for) uint16_t.  This forces
> the host compiler to assume garbage in the upper 16 bits on input
> and to zero-extend the result on output.
>
> Signed-off-by: Richard Henderson 

Some AArch64 tests I had that previously failed on a x86-64 host now pass.

Tested-by: Laurent Desnogues 

Perhaps the two occurrences of the following comment in
target/arm/translate-a64.c could be removed along with this patch:

/* write_fp_sreg is OK here because top half of tcg_rd is zero */

or reworded.

Thanks,

Laurent

> ---
>  include/exec/helper-head.h |  2 +-
>  target/arm/helper-a64.c| 35 +
>  target/arm/helper.c| 80 +++---
>  3 files changed, 59 insertions(+), 58 deletions(-)
>
> diff --git a/include/exec/helper-head.h b/include/exec/helper-head.h
> index 15b6a68de3..276dd5afce 100644
> --- a/include/exec/helper-head.h
> +++ b/include/exec/helper-head.h
> @@ -39,7 +39,7 @@
>  #define dh_ctype_int int
>  #define dh_ctype_i64 uint64_t
>  #define dh_ctype_s64 int64_t
> -#define dh_ctype_f16 float16
> +#define dh_ctype_f16 uint32_t
>  #define dh_ctype_f32 float32
>  #define dh_ctype_f64 float64
>  #define dh_ctype_ptr void *
> diff --git a/target/arm/helper-a64.c b/target/arm/helper-a64.c
> index f92bdea732..6ee5f684cf 100644
> --- a/target/arm/helper-a64.c
> +++ b/target/arm/helper-a64.c
> @@ -85,12 +85,12 @@ static inline uint32_t float_rel_to_flags(int res)
>  return flags;
>  }
>
> -uint64_t HELPER(vfp_cmph_a64)(float16 x, float16 y, void *fp_status)
> +uint64_t HELPER(vfp_cmph_a64)(uint32_t x, uint32_t y, void *fp_status)
>  {
>  return float_rel_to_flags(float16_compare_quiet(x, y, fp_status));
>  }
>
> -uint64_t HELPER(vfp_cmpeh_a64)(float16 x, float16 y, void *fp_status)
> +uint64_t HELPER(vfp_cmpeh_a64)(uint32_t x, uint32_t y, void *fp_status)
>  {
>  return float_rel_to_flags(float16_compare(x, y, fp_status));
>  }
> @@ -214,7 +214,7 @@ uint64_t HELPER(neon_cgt_f64)(float64 a, float64 b, void 
> *fpstp)
>  #define float64_three make_float64(0x4008ULL)
>  #define float64_one_point_five make_float64(0x3FF8ULL)
>
> -float16 HELPER(recpsf_f16)(float16 a, float16 b, void *fpstp)
> +uint32_t HELPER(recpsf_f16)(uint32_t a, uint32_t b, void *fpstp)
>  {
>  float_status *fpst = fpstp;
>
> @@ -259,7 +259,7 @@ float64 HELPER(recpsf_f64)(float64 a, float64 b, void 
> *fpstp)
>  return float64_muladd(a, b, float64_two, 0, fpst);
>  }
>
> -float16 HELPER(rsqrtsf_f16)(float16 a, float16 b, void *fpstp)
> +uint32_t HELPER(rsqrtsf_f16)(uint32_t a, uint32_t b, void *fpstp)
>  {
>  float_status *fpst = fpstp;
>
> @@ -366,7 +366,7 @@ uint64_t HELPER(neon_addlp_u16)(uint64_t a)
>  }
>
>  /* Floating-point reciprocal exponent - see FPRecpX in ARM ARM */
> -float16 HELPER(frecpx_f16)(float16 a, void *fpstp)
> +uint32_t HELPER(frecpx_f16)(uint32_t a, void *fpstp)
>  {
>  float_status *fpst = fpstp;
>  uint16_t val16, sbit;
> @@ -695,7 +695,7 @@ void HELPER(casp_be_parallel)(CPUARMState *env, uint32_t 
> rs, uint64_t addr,
>  #define ADVSIMD_HELPER(name, suffix) HELPER(glue(glue(advsimd_, name), 
> suffix))
>
>  #define ADVSIMD_HALFOP(name) \
> -float16 ADVSIMD_HELPER(name, h)(float16 a, float16 b, void *fpstp) \
> +uint32_t ADVSIMD_HELPER(name, h)(uint32_t a, uint32_t b, void *fpstp) \
>  { \
>  float_status *fpst = fpstp; \
>  return float16_ ## name(a, b, fpst);\
> @@ -755,7 +755,8 @@ ADVSIMD_HALFOP(mulx)
>  ADVSIMD_TWOHALFOP(mulx)
>
>  /* fused multiply-accumulate */
> -float16 HELPER(advsimd_muladdh)(float16 a, float16 b, float16 c, void *fpstp)
> +uint32_t HELPER(advsimd_muladdh)(uint32_t a, uint32_t b, uint32_t c,
> + void *fpstp)
>  {
>  float_status *fpst = fpstp;
>  return float16_muladd(a, b, c, 0, fpst);
> @@ -786,14 +787,14 @@ uint32_t HELPER(advsimd_muladd2h)(uint32_t two_a, 
> uint32_t two_b,
>
>  #define ADVSIMD_CMPRES(test) (test) ? 0x : 0
>
> -uint32_t HELPER(advsimd_ceq_f16)(float16 a, float16 b, void *fpstp)
> +uint32_t HELPER(advsimd_ceq_f16)(uint32_t a, uint32_t b, void *fpstp)
>  {
>  float_status *fpst = fpstp;
>  int compare = float16_compare_quiet(a, b, fpst);
>  return

Re: [Qemu-devel] [PATCH] tcg: Fix helper function vs host abi for float16

2018-05-22 Thread no-reply 
Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20180522175629.24932-1-richard.hender...@linaro.org
Subject: [Qemu-devel] [PATCH] tcg: Fix helper function vs host abi for float16

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 t [tag update]
patchew/20180521140402.23318-1-peter.mayd...@linaro.org -> 
patchew/20180521140402.23318-1-peter.mayd...@linaro.org
 * [new tag]   
patchew/20180522175629.24932-1-richard.hender...@linaro.org -> 
patchew/20180522175629.24932-1-richard.hender...@linaro.org
Switched to a new branch 'test'
b59013060a tcg: Fix helper function vs host abi for float16

=== OUTPUT BEGIN ===
Checking PATCH 1/1: tcg: Fix helper function vs host abi for float16...
ERROR: space prohibited before that close parenthesis ')'
#233: FILE: target/arm/helper.c:11367:
+CONV_FTOI(vfp_to##name##p, ftype, fsz, sign, )\

ERROR: space prohibited before that close parenthesis ')'
#242: FILE: target/arm/helper.c:11370:
+FLOAT_CONVS(si, h, uint32_t, 16, )

ERROR: space prohibited before that close parenthesis ')'
#243: FILE: target/arm/helper.c:11371:
+FLOAT_CONVS(si, s, float32, 32, )

ERROR: space prohibited before that close parenthesis ')'
#244: FILE: target/arm/helper.c:11372:
+FLOAT_CONVS(si, d, float64, 64, )

total: 4 errors, 0 warnings, 312 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-de...@redhat.com

[Qemu-devel] [PATCH] tcg: Fix helper function vs host abi for float16

2018-05-22 Thread Richard Henderson 
Depending on the host abi, float16, aka uint16_t, values are
passed and returned either zero-extended in the host register
or with garbage at the top of the host register.

The tcg code generator has so far been assuming garbage, as that
matches the x86 abi, but this is incorrect for other host abis.
Further, target/arm has so far been assuming zero-extended results,
so that it may store the 16-bit value into a 32-bit slot with the
high 16-bits already clear.

Rectify both problems by mapping "f16" in the helper definition
to uint32_t instead of (a typedef for) uint16_t.  This forces
the host compiler to assume garbage in the upper 16 bits on input
and to zero-extend the result on output.

Signed-off-by: Richard Henderson 
---
 include/exec/helper-head.h |  2 +-
 target/arm/helper-a64.c| 35 +
 target/arm/helper.c| 80 +++---
 3 files changed, 59 insertions(+), 58 deletions(-)

diff --git a/include/exec/helper-head.h b/include/exec/helper-head.h
index 15b6a68de3..276dd5afce 100644
--- a/include/exec/helper-head.h
+++ b/include/exec/helper-head.h
@@ -39,7 +39,7 @@
 #define dh_ctype_int int
 #define dh_ctype_i64 uint64_t
 #define dh_ctype_s64 int64_t
-#define dh_ctype_f16 float16
+#define dh_ctype_f16 uint32_t
 #define dh_ctype_f32 float32
 #define dh_ctype_f64 float64
 #define dh_ctype_ptr void *
diff --git a/target/arm/helper-a64.c b/target/arm/helper-a64.c
index f92bdea732..6ee5f684cf 100644
--- a/target/arm/helper-a64.c
+++ b/target/arm/helper-a64.c
@@ -85,12 +85,12 @@ static inline uint32_t float_rel_to_flags(int res)
 return flags;
 }
 
-uint64_t HELPER(vfp_cmph_a64)(float16 x, float16 y, void *fp_status)
+uint64_t HELPER(vfp_cmph_a64)(uint32_t x, uint32_t y, void *fp_status)
 {
 return float_rel_to_flags(float16_compare_quiet(x, y, fp_status));
 }
 
-uint64_t HELPER(vfp_cmpeh_a64)(float16 x, float16 y, void *fp_status)
+uint64_t HELPER(vfp_cmpeh_a64)(uint32_t x, uint32_t y, void *fp_status)
 {
 return float_rel_to_flags(float16_compare(x, y, fp_status));
 }
@@ -214,7 +214,7 @@ uint64_t HELPER(neon_cgt_f64)(float64 a, float64 b, void 
*fpstp)
 #define float64_three make_float64(0x4008ULL)
 #define float64_one_point_five make_float64(0x3FF8ULL)
 
-float16 HELPER(recpsf_f16)(float16 a, float16 b, void *fpstp)
+uint32_t HELPER(recpsf_f16)(uint32_t a, uint32_t b, void *fpstp)
 {
 float_status *fpst = fpstp;
 
@@ -259,7 +259,7 @@ float64 HELPER(recpsf_f64)(float64 a, float64 b, void 
*fpstp)
 return float64_muladd(a, b, float64_two, 0, fpst);
 }
 
-float16 HELPER(rsqrtsf_f16)(float16 a, float16 b, void *fpstp)
+uint32_t HELPER(rsqrtsf_f16)(uint32_t a, uint32_t b, void *fpstp)
 {
 float_status *fpst = fpstp;
 
@@ -366,7 +366,7 @@ uint64_t HELPER(neon_addlp_u16)(uint64_t a)
 }
 
 /* Floating-point reciprocal exponent - see FPRecpX in ARM ARM */
-float16 HELPER(frecpx_f16)(float16 a, void *fpstp)
+uint32_t HELPER(frecpx_f16)(uint32_t a, void *fpstp)
 {
 float_status *fpst = fpstp;
 uint16_t val16, sbit;
@@ -695,7 +695,7 @@ void HELPER(casp_be_parallel)(CPUARMState *env, uint32_t 
rs, uint64_t addr,
 #define ADVSIMD_HELPER(name, suffix) HELPER(glue(glue(advsimd_, name), suffix))
 
 #define ADVSIMD_HALFOP(name) \
-float16 ADVSIMD_HELPER(name, h)(float16 a, float16 b, void *fpstp) \
+uint32_t ADVSIMD_HELPER(name, h)(uint32_t a, uint32_t b, void *fpstp) \
 { \
 float_status *fpst = fpstp; \
 return float16_ ## name(a, b, fpst);\
@@ -755,7 +755,8 @@ ADVSIMD_HALFOP(mulx)
 ADVSIMD_TWOHALFOP(mulx)
 
 /* fused multiply-accumulate */
-float16 HELPER(advsimd_muladdh)(float16 a, float16 b, float16 c, void *fpstp)
+uint32_t HELPER(advsimd_muladdh)(uint32_t a, uint32_t b, uint32_t c,
+ void *fpstp)
 {
 float_status *fpst = fpstp;
 return float16_muladd(a, b, c, 0, fpst);
@@ -786,14 +787,14 @@ uint32_t HELPER(advsimd_muladd2h)(uint32_t two_a, 
uint32_t two_b,
 
 #define ADVSIMD_CMPRES(test) (test) ? 0x : 0
 
-uint32_t HELPER(advsimd_ceq_f16)(float16 a, float16 b, void *fpstp)
+uint32_t HELPER(advsimd_ceq_f16)(uint32_t a, uint32_t b, void *fpstp)
 {
 float_status *fpst = fpstp;
 int compare = float16_compare_quiet(a, b, fpst);
 return ADVSIMD_CMPRES(compare == float_relation_equal);
 }
 
-uint32_t HELPER(advsimd_cge_f16)(float16 a, float16 b, void *fpstp)
+uint32_t HELPER(advsimd_cge_f16)(uint32_t a, uint32_t b, void *fpstp)
 {
 float_status *fpst = fpstp;
 int compare = float16_compare(a, b, fpst);
@@ -801,14 +802,14 @@ uint32_t HELPER(advsimd_cge_f16)(float16 a, float16 b, 
void *fpstp)
   compare == float_relation_equal);
 }
 
-uint32_t HELPER(advsimd_cgt_f16)(float16 a, float16 b, void *fpstp)
+uint32_t HELPER(advsimd_cgt_f16)(uint32_t a, uint32_t b, void *fpstp)
 {
 float_status *fpst = fpstp;
 int compare = float16_compare(a, b, fpst);
 return ADVSIMD_CMPRES(compare