Re: [PATCH] target/arm: Fix sve pred_desc decoding

[Peter Maydell]

On Wed, 30 Dec 2020 at 17:25, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> There was an inconsistency between encoding, which uses
> SIMD_DATA_SHIFT, and decoding which used SIMD_OPRSZ_BITS.
> This happened to be ok, until e2e7168a214, which reduced
> the size of SIMD_OPRSZ_BITS, which lead to truncating all
> predicate vector lengths.
>
> Cc: qemu-stable@nongnu.org
> Buglink: https://bugs.launchpad.net/bugs/1908551
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>
> Ouch.  The patch that exposed this, e2e7168a214, went in near
> the start of the 5.2 devel period, and I never noticed.  I've
> been doing most of my testing vs ArmIE of late, which due to
> lack of a proper sve signal frame restricts RISU to sve128,
> which worked fine with this truncation.  I need to spend some
> time getting FVP working again...
>
>
> r~
>
> ---
>  target/arm/sve_helper.c | 46 ++++++++++++++++++++---------------------
>  1 file changed, 23 insertions(+), 23 deletions(-)
>
> diff --git a/target/arm/sve_helper.c b/target/arm/sve_helper.c
> index 5f037c3a8f..99e4b70d2f 100644
> --- a/target/arm/sve_helper.c
> +++ b/target/arm/sve_helper.c
> @@ -914,7 +914,7 @@ uint32_t HELPER(sve_pfirst)(void *vd, void *vg, uint32_t 
> words)
>
>  uint32_t HELPER(sve_pnext)(void *vd, void *vg, uint32_t pred_desc)
>  {
> -    intptr_t words = extract32(pred_desc, 0, SIMD_OPRSZ_BITS);
> +    intptr_t words = extract32(pred_desc, 0, SIMD_DATA_SHIFT);
>      intptr_t esz = extract32(pred_desc, SIMD_DATA_SHIFT, 2);
>      uint32_t flags = PREDTEST_INIT;
>      uint64_t *d = vd, *g = vg, esz_mask;
> @@ -1867,7 +1867,7 @@ static uint64_t compress_bits(uint64_t x, int n)
>
>  void HELPER(sve_zip_p)(void *vd, void *vn, void *vm, uint32_t pred_desc)
>  {
> -    intptr_t oprsz = extract32(pred_desc, 0, SIMD_OPRSZ_BITS) + 2;
> +    intptr_t oprsz = extract32(pred_desc, 0, SIMD_DATA_SHIFT) + 2;

Why do we not get oprsz by extracting SIMD_OPRSZ_BITS starting at
SIMD_OPRSZ_SHIFT ? (or even by calling simd_oprsz(), which
certainly looks like it ought to be a helper function for
extracting the oprsz...)

If the encoding constants in tcg-gvec-desc.h are right then
"SIMD_DATA_SHIFT bits starting at bit 0" is two fields glued
together (MAXSZ and OPRSZ).

thanks
-- PMM