qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Richard Henderson
Subject: Re: [RFC PATCH v3 14/34] Hexagon (target/hexagon) instruction/packet decode
Date: Wed, 26 Aug 2020 08:06:31 -0700
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.10.0

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~



reply via email to

[Prev in Thread] Current Thread [Next in Thread]