[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~
- Re: [RFC PATCH v3 11/34] Hexagon (target/hexagon) register fields, (continued)
- [RFC PATCH v3 02/34] Hexagon (target/hexagon) README, Taylor Simpson, 2020/08/18
- [RFC PATCH v3 03/34] Hexagon (include/elf.h) ELF machine definition, Taylor Simpson, 2020/08/18
- [RFC PATCH v3 13/34] Hexagon (target/hexagon) register map, Taylor Simpson, 2020/08/18
- [RFC PATCH v3 14/34] Hexagon (target/hexagon) instruction/packet decode, Taylor Simpson, 2020/08/18
- Re: [RFC PATCH v3 14/34] Hexagon (target/hexagon) instruction/packet decode,
Richard Henderson <=
- [RFC PATCH v3 07/34] Hexagon (target/hexagon) scalar core helpers, Taylor Simpson, 2020/08/18
- [RFC PATCH v3 17/34] Hexagon (target/hexagon/imported) arch import - macro definitions, Taylor Simpson, 2020/08/18
- [RFC PATCH v3 23/34] Hexagon (target/hexagon) generater phase 4 - decode tree, Taylor Simpson, 2020/08/18
- [RFC PATCH v3 09/34] Hexagon (target/hexagon) architecture types, Taylor Simpson, 2020/08/18