Re: [PATCH v2 00/57] target/arm: First slice of MVE implementation

[Peter Maydell]

On Mon, 14 Jun 2021 at 23:22, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 6/14/21 8:09 AM, Peter Maydell wrote:
> >      - pass only ESIZE, not H, to macros in mve_helper.c
>
> I've been thinking about the H* macros again while reading this.
>
> While H##ESIZE is an improvement over passing in HN, I think we can do better 
> and force
> the adjustment to match the type -- completely avoiding bugs you've caught at 
> least twice
> during SVE review.
>
> The form I'm playing with today is
>
> #ifdef HOST_WORDS_BIGENDIAN
> #define HTADJ(p) (7 >> __builtin_ctz(sizeof(*(p))))
> #define HBADJ(p) (7 & (7 << __builtin_ctz(sizeof(*(p)))))
> #else
> #define HTADJ(p) 0
> #define HBADJ(p) 0
> #endif
>
> /**
>   * HT: adjust Host addressing by Type
>   * @p: data pointer
>   * @i: array index
>   *
>   * Adjust p[i] for host-endian addressing of sub-quadword values.
>   */
> #define HT(p, i)  ((p)[(i) ^ HADJ(p)])
>
> /**
>   * HB: adjust Host addressing by Bype
>   * @p: data pointer
>   * @i: byte offset
>   *
>   * Adjust p[i / sizeof(*p)] for host-endian addressing
>   * of sub-quadword values.  Unlike HT, @i is not an array
>   * index but a byte offset.
>   */
> #define HB(p, i) (*(__typeof(p))((uintptr_t)(p) + ((i) ^ H1ADJ(p))))
>
> void bt(unsigned char  *x, long i) { H(x, i) = 0; }
> void ht(unsigned short *x, long i) { H(x, i) = 0; }
> void wt(unsigned int   *x, long i) { H(x, i) = 0; }
> void qt(unsigned long  *x, long i) { H(x, i) = 0; }
>
> void bb(unsigned char  *x, long i) { H1(x, i) = 0; }
> void hb(unsigned short *x, long i) { H1(x, i) = 0; }
> void wb(unsigned int   *x, long i) { H1(x, i) = 0; }
> void qb(unsigned long  *x, long i) { H1(x, i) = 0; }

What are these functions for ?

> This gives us
>
> #define DO_1OP(OP, TYPE, FN)                                            \
>      void HELPER(mve_##OP)(CPUARMState *env, void *vd, void *vm)         \
>      {                                                                   \
>          TYPE *d = vd, *m = vm;                                          \
>          uint16_t mask = mve_element_mask(env);                          \
>          unsigned e;                                                     \
>          unsigned const esize = sizeof(TYPE);                            \
>          for (e = 0; e < 16 / esize; e++, mask >>= esize) {              \
>              mergemask(&HT(d, e), FN(HT(m, e)]), mask);                  \
>          }                                                               \
>          mve_advance_vpt(env);                                           \
>      }
>
> Thoughts?  Especially on the naming of the macros?
> If the idea appeals, I'll do a pass over the existing code.

Getting rid of the need to keep matches between H macros and
the types certainly sounds like a good idea. I don't have a strong
view on the macro names -- they're always going to be a bit opaque
because we want to give them short names.

-- PMM