On Mon, 9 Sept 2024 at 17:25, Richard Henderson
<richard.hender...@linaro.org> wrote:
>
> While these functions really do return a 32-bit value,
> widening the return type means that we need do less
> marshalling between TCG types.
>
> Remove NeonGenNarrowEnvFn typedef; add NeonGenOne64OpEnvFn.
>
> Signed-off-by: Richard Henderson <richard.hender...@linaro.org>
> --- a/target/arm/tcg/neon_helper.c
> +++ b/target/arm/tcg/neon_helper.c
> @@ -598,13 +598,13 @@ NEON_VOP_ENV(qrdmulh_s32, neon_s32, 1)
> #undef NEON_FN
> #undef NEON_QDMULH32
>
> -uint32_t HELPER(neon_narrow_u8)(uint64_t x)
> +uint64_t HELPER(neon_narrow_u8)(uint64_t x)
> {
> return (x & 0xffu) | ((x >> 8) & 0xff00u) | ((x >> 16) & 0xff0000u)
> | ((x >> 24) & 0xff000000u);
> }
For almost all of these helper functions where we change the return
type, we return the same value we used to return, but zero extended
from 32 to 64 bits.
> -uint32_t HELPER(neon_narrow_sat_s32)(CPUARMState *env, uint64_t x)
> +uint64_t HELPER(neon_narrow_sat_s32)(CPUARMState *env, uint64_t x)
> {
> if ((int64_t)x != (int32_t)x) {
> SET_QC();
But for this one we don't. e.g. if the input is 0xffff0000ffff0000
then the old output was 0x80000000 and the new output is
0xffffffff80000000.
Presumably we're effectively ignoring the high 32 bits in
the caller, but this seems inconsistent.
It might also be helpful to have a comment describing what
the semantics of the return value is for this class of helpers.
I suspect also that coverity may complain about the
return (uint16_t)low | (high << 16);
in neon_unarrow_sat16, neon_narrow_sat_u16 and neon_narrow_sat_s16,
which have the desired semantics but look suspiciously like
somebody forgot to do the shift of 'high' as a 64-bit shift.
I guess we could write them as
return deposit32(low, 16, 16, high);
or maybe
return (uint16_t)low | (uint32_t)(high << 16);
to avoid that.
thanks
-- PMM
