Re: [PATCH v5 25/45] target/arm: Implement BFMOPA, BFMOPS

[Peter Maydell <peter.maydell@linaro.org>]

On Wed, 6 Jul 2022 at 10:36, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/arm/helper-sme.h    |  2 ++
>  target/arm/sme.decode      |  2 ++
>  target/arm/sme_helper.c    | 52 ++++++++++++++++++++++++++++++++++++++
>  target/arm/translate-sme.c | 30 ++++++++++++++++++++++
>  4 files changed, 86 insertions(+)
>
> diff --git a/target/arm/helper-sme.h b/target/arm/helper-sme.h
> index f50d0fe1d6..1d68fb8c74 100644
> --- a/target/arm/helper-sme.h
> +++ b/target/arm/helper-sme.h
> @@ -125,3 +125,5 @@ DEF_HELPER_FLAGS_7(sme_fmopa_s, TCG_CALL_NO_RWG,
>                     void, ptr, ptr, ptr, ptr, ptr, ptr, i32)
>  DEF_HELPER_FLAGS_7(sme_fmopa_d, TCG_CALL_NO_RWG,
>                     void, ptr, ptr, ptr, ptr, ptr, ptr, i32)
> +DEF_HELPER_FLAGS_6(sme_bfmopa, TCG_CALL_NO_RWG,
> +                   void, ptr, ptr, ptr, ptr, ptr, i32)
> diff --git a/target/arm/sme.decode b/target/arm/sme.decode
> index ba4774d174..afd9c0dffd 100644
> --- a/target/arm/sme.decode
> +++ b/target/arm/sme.decode
> @@ -73,3 +73,5 @@ ADDVA_d         11000000 11 01000 1 ... ... ..... 00 ...        @adda_64
>
>  FMOPA_s         10000000 100 ..... ... ... ..... . 00 ..        @op_32
>  FMOPA_d         10000000 110 ..... ... ... ..... . 0 ...        @op_64
> +
> +BFMOPA          10000001 100 ..... ... ... ..... . 00 ..        @op_32
> diff --git a/target/arm/sme_helper.c b/target/arm/sme_helper.c
> index 78ba34f3d2..4b437bb913 100644
> --- a/target/arm/sme_helper.c
> +++ b/target/arm/sme_helper.c
> @@ -981,3 +981,55 @@ void HELPER(sme_fmopa_d)(void *vza, void *vzn, void *vzm, void *vpn,
>          }
>      }
>  }
> +
> +/*
> + * Alter PAIR as needed for controlling predicates being false,
> + * and for NEG on an enabled row element.
> + */
> +static inline uint32_t f16mop_adj_pair(uint32_t pair, uint32_t pg, uint32_t neg)
> +{
> +    pair ^= neg;

You seem to be negating element 1 of row and col ('neg' here is
1 << 15 unless I've misread something, and it gets passed to
the calls for both the row and column data), but the pseudocode
says we want to negate element 0 and element 1 of row, and not
negate the col elements.

> +    if (!(pg & 1)) {
> +        pair &= 0xffff0000u;
> +    }
> +    if (!(pg & 4)) {
> +        pair &= 0x0000ffffu;
> +    }

The pseudocode sets the element to 0 if it is not
predicated, and then applies the negation second.

> +    return pair;
> +}
> +
> +void HELPER(sme_bfmopa)(void *vza, void *vzn, void *vzm, void *vpn,
> +                        void *vpm, uint32_t desc)
> +{
> +    intptr_t row, col, oprsz = simd_maxsz(desc);
> +    uint32_t neg = simd_data(desc) << 15;
> +    uint16_t *pn = vpn, *pm = vpm;
> +
> +    for (row = 0; row < oprsz; ) {
> +        uint16_t pa = pn[H2(row >> 4)];
> +        do {
> +            void *vza_row = vza + tile_vslice_offset(row);
> +            uint32_t n = *(uint32_t *)(vzn + row);

More missing H macros ?

> +
> +            n = f16mop_adj_pair(n, pa, neg);
> +
> +            for (col = 0; col < oprsz; ) {
> +                uint16_t pb = pm[H2(col >> 4)];
> +                do {
> +                    if ((pa & 0b0101) == 0b0101 || (pb & 0b0101) == 0b0101) {

The pseudocode test for "do we do anything" is
 (prow_0 && pcol_0) || (prow_1 && pcol_1)

but isn't this C expression doing
 (prow_0 && prow_1) || (pcol_0 && pcol_1) ?

> +                        uint32_t *a = vza_row + col;
> +                        uint32_t m = *(uint32_t *)(vzm + col);
> +
> +                        m = f16mop_adj_pair(m, pb, neg);
> +                        *a = bfdotadd(*a, n, m);
> +
> +                        col += 4;
> +                        pb >>= 4;
> +                    }
> +                } while (col & 15);
> +            }
> +            row += 4;
> +            pa >>= 4;
> +        } while (row & 15);
> +    }
> +}

thanks
-- PMM