Re: [RFC PATCH v3 24/34] Hexagon (target/hexagon) opcode data structures

[Richard Henderson]

On 8/26/20 4:52 PM, Taylor Simpson wrote:
>> And using qemu/bitops.h if possible, as discussed earlier vs attribs.h.
> 
> Do you mean replace the GET_ATTRIB macro with test_bit from qemu/bitops.h?

No, just define GET_ATTRIB in terms of test_bit, and define opcode_attribs
using BITS_TO_LONGS.


r~

RE: [RFC PATCH v3 24/34] Hexagon (target/hexagon) opcode data structures

[Taylor Simpson]

> -Original Message-
> From: Richard Henderson 
> Sent: Wednesday, August 26, 2020 9:26 AM
> To: Taylor Simpson ; qemu-devel@nongnu.org
> Cc: phi...@redhat.com; laur...@vivier.eu; riku.voi...@iki.fi;
> aleksandar.m.m...@gmail.com; a...@rev.ng
> Subject: Re: [RFC PATCH v3 24/34] Hexagon (target/hexagon) opcode data
> structures
>
> > +extern const char *opcode_names[];
> > +
> > +extern const char *opcode_reginfo[];
> > +extern const char *opcode_rregs[];
> > +extern const char *opcode_wregs[];
>
> const char * const

OK

> > +extern opcode_encoding_t opcode_encodings[XX_LAST_OPCODE];
>
> const.

OK

> > +extern size4u_t
> > +opcode_attribs[XX_LAST_OPCODE][(A_ZZ_LASTATTRIB /
> ATTRIB_WIDTH) + 1];
>
> const.

OK

> And using qemu/bitops.h if possible, as discussed earlier vs attribs.h.

Do you mean replace the GET_ATTRIB macro with test_bit from qemu/bitops.h?

> Just initialize opcode_short_semantics with shortcode_generated.h in the
> first
> place.  Then you don't need to create a table of NULL and overwrite at
> startup.
>
> And you can also make the table constant.

OK

> > +if (p == NULL) {
> > +g_assert_not_reached();
> > +return 0;
> > +}
>
> I prefer assert(p != NULL) to if (test) { abort(); }, where possible.  E.g.
> this later one is fine:

OK

Re: [RFC PATCH v3 24/34] Hexagon (target/hexagon) opcode data structures

[Richard Henderson]

On 8/18/20 8:50 AM, Taylor Simpson wrote:
> +extern const char *opcode_names[];
> +
> +extern const char *opcode_reginfo[];
> +extern const char *opcode_rregs[];
> +extern const char *opcode_wregs[];

const char * const


> +extern opcode_encoding_t opcode_encodings[XX_LAST_OPCODE];

const.

> +extern size4u_t
> +opcode_attribs[XX_LAST_OPCODE][(A_ZZ_LASTATTRIB / ATTRIB_WIDTH) + 1];

const.

And using qemu/bitops.h if possible, as discussed earlier vs attribs.h.

> +const char *opcode_short_semantics[] = {
> +#define OPCODE(X)  NULL
> +#include "opcodes_def_generated.h"
> +#undef OPCODE
> +NULL
> +};
...
> +#define DEF_SHORTCODE(TAG, SHORTCODE) \
> +opcode_short_semantics[TAG] = #SHORTCODE;
> +#include "shortcode_generated.h"
> +#undef DEF_SHORTCODE

Just initialize opcode_short_semantics with shortcode_generated.h in the first
place.  Then you don't need to create a table of NULL and overwrite at startup.

And you can also make the table constant.

> +if (p == NULL) {
> +g_assert_not_reached();
> +return 0;
> +}

I prefer assert(p != NULL) to if (test) { abort(); }, where possible.  E.g.
this later one is fine:

> +if (islower(*p)) {
> +return 0;
> +} else if (isupper(*p)) {
> +return 1;
> +} else {
> +g_assert_not_reached();
> +}


r~