RE: [RFC PATCH v3 27/34] Hexagon (target/hexagon) instruction classes

[Taylor Simpson]

> -----Original Message-----
> From: Richard Henderson <richard.henderson@linaro.org>
> Sent: Friday, August 28, 2020 7:37 PM
> To: Taylor Simpson <tsimpson@quicinc.com>; qemu-devel@nongnu.org
> Cc: philmd@redhat.com; laurent@vivier.eu; riku.voipio@iki.fi;
> aleksandar.m.mail@gmail.com; ale@rev.ng
> Subject: Re: [RFC PATCH v3 27/34] Hexagon (target/hexagon) instruction
> classes
>
> On 8/18/20 8:50 AM, Taylor Simpson wrote:
> > +} iclass_t;
> ...
> > +extern const char *find_iclass_slots(opcode_t opcode, int itype);
> ...
> > +typedef struct {
> > +    const char * const slots;
> > +} iclass_info_t;
>
> I'll note that you aren't following our CODING_STYLE for types.  Which of
> these
> need to match imported/ and which can you fix.

So, this should be CamelCase?  I should be able to fix all of them.

>
> > +typedef enum {
> > +
> > +#define DEF_PP_ICLASS32(TYPE, SLOTS, UNITS)
> ICLASS_FROM_TYPE(TYPE),
> > +#define DEF_EE_ICLASS32(TYPE, SLOTS, UNITS)    /* nothing */
> > +#include "imported/iclass.def"
> > +#undef DEF_PP_ICLASS32
> > +#undef DEF_EE_ICLASS32
> > +
> > +#define DEF_EE_ICLASS32(TYPE, SLOTS, UNITS)
> ICLASS_FROM_TYPE(TYPE),
> > +#define DEF_PP_ICLASS32(TYPE, SLOTS, UNITS)    /* nothing */
> > +#include "imported/iclass.def"
> > +#undef DEF_PP_ICLASS32
> > +#undef DEF_EE_ICLASS32
>
> Any particular reason why you're defining one as nothing, and doing this
> dance
> twice?

Will investigate.

> > +const char *find_iclass_slots(opcode_t opcode, int itype)
> > +{
> > +    /* There are some exceptions to what the iclass dictates */
> > +    if (GET_ATTRIB(opcode, A_ICOP)) {
> > +        return "2";
>
> Why are you returning a string and not a simple bitmask?

Will fix

> > +    [ICLASS_FROM_TYPE(TYPE)] = { .slots = #SLOTS },
>
> This could be converted to the bitmask with
>
> enum {
>     SLOTS_0  = (1 << 0),
>     SLOTS_1  = (1 << 1),
>     SLOTS_23 = (1 << 2) | (1 << 3),
>     ...
> };
>
> static const uint8_t iclass_info[] = {
>
> #define DEF_ICLASS(TYPE, SLOTS) \
>     [ICLASS_##TYPE] = SLOTS_##SLOTS
> #define DEF_PP_ICLASS32(TYPE, SLOTS, UNITS) \
>     DEF_ICLASS(TYPE, SLOTS)
> #define DEF_EE_ICLASS32(TYPE, SLOTS, UNITS) \
>     DEF_ICLASS(TYPE, SLOTS)
>
> At which point the ultimate consumer,
>
> >     for (i = 0, slot = 3; i < pkt->num_insns; i++) {
> >         valid_slot_str = get_valid_slot_str(pkt, i);
> >
> >         while (strchr(valid_slot_str, '0' + slot) == NULL) {
> >             slot--;
> >         }
> >         pkt->insn[i].slot = slot;
>
> becomes a simple integer mask check.

Will fix