Re: [RFC PATCH v3 14/34] Hexagon (target/hexagon) instruction/packet decode

[Richard Henderson <richard.henderson@linaro.org>]

On 8/18/20 8:50 AM, Taylor Simpson wrote:
> +#define DECODE_NEW_TABLE(TAG, SIZE, WHATNOT) \
> +    static struct _dectree_table_struct dectree_table_##TAG;

All of these little structures should be const.


> +typedef struct {
> +    struct _dectree_table_struct *table_link;
> +    struct _dectree_table_struct *table_link_b;
> +    opcode_t opcode;
> +    enum {
> +        DECTREE_ENTRY_INVALID,
> +        DECTREE_TABLE_LINK,
> +        DECTREE_SUBINSNS,
> +        DECTREE_EXTSPACE,
> +        DECTREE_TERMINAL
> +    } type;
> +} dectree_entry_t;

Which probably requires these links to be const, and a few uses within the
functions that use them.

That should move 78K worth of tables into .data.rel.ro, where they can't be
modified after relocation.

> +            /* previous insn is the producer */
> +            def_opcode = packet->insn[def_idx].opcode;
> +            dststr = strstr(opcode_wregs[def_opcode], "Rd");
> +            if (dststr) {
> +                dststr = strchr(opcode_reginfo[def_opcode], 'd');
> +            } else {
> +                dststr = strstr(opcode_wregs[def_opcode], "Rx");
> +                if (dststr) {
> +                    dststr = strchr(opcode_reginfo[def_opcode], 'x');
> +                } else {
> +                    dststr = strstr(opcode_wregs[def_opcode], "Re");
> +                    if (dststr) {
> +                        dststr = strchr(opcode_reginfo[def_opcode], 'e');
> +                    } else {
> +                        dststr = strstr(opcode_wregs[def_opcode], "Ry");
> +                        if (dststr) {
> +                            dststr = strchr(opcode_reginfo[def_opcode], 'y');
> +                        } else {
> +                            g_assert_not_reached();
> +                            return 1;
> +                        }
> +                    }
> +                }
> +            }

I can't help but thinking that all of this string manipulation is not the most
ideal way to implement a decoder.  Surely all this can be pre-processed into
code, or flags, or an enumeration or something.

Oh, and g_assert_not_reached() will never return.  You don't have to keep
putting return statements after it.  Please remove all of those, everywhere.

I'm going to ignore the rest of the decoder, as it's somewhat bewildering.
Even in the final patches-applied form.  It could really use some more commentary.


r~