[PATCH 5/6] aarch64: Use an enum to refer to indices in the opcode table
Alice Carlotti
alice.carlotti@arm.com
Fri Jul 18 15:34:19 GMT 2025
On Fri, Jul 18, 2025 at 11:04:00AM +0100, Richard Earnshaw wrote:
> The indices into the auto-generated tables for opcodes are relatively
> unstable. Adding a new opcode can permute the code significantly.
> But most of this churn is down to changes in the index values. To
> minimize this use enumerated constants. While the index values
> change, the enumeration names will need to do so far less often, so
> most of the changes in the generated code become localized to the
> addition (occasionally removal) of opcodes.
I like this change, both for the stability but also because it will make the
generated files much more human-readable. I hadn't appreciated that second
benefit until seeing the new generated patches, but it would have been helpful
on the many occasions that I've looked at what the generated functions do.
>
> The enumeration values are placed in a new header file, aarch64-tbl-2.h,
> so aarch64-gen gains a new option to build this header and the Makefile
> rules are adjusted accordingly.
> ---
> opcodes/Makefile.am | 10 ++-
> opcodes/aarch64-gen.c | 197 +++++++++++++++++++++++++++++++++---------
> 2 files changed, 164 insertions(+), 43 deletions(-)
>
> diff --git a/opcodes/Makefile.am b/opcodes/Makefile.am
> index ef4b0dfb01d..a8642c4d07e 100644
> --- a/opcodes/Makefile.am
> +++ b/opcodes/Makefile.am
> @@ -492,7 +492,7 @@ MOSTLYCLEANFILES = aarch64-gen$(EXEEXT_FOR_BUILD) i386-gen$(EXEEXT_FOR_BUILD) \
> z8kgen$(EXEEXT_FOR_BUILD) opc2c$(EXEEXT_FOR_BUILD)
>
> MAINTAINERCLEANFILES = $(srcdir)/aarch64-asm-2.c $(srcdir)/aarch64-dis-2.c \
> - $(srcdir)/aarch64-opc-2.c \
> + $(srcdir)/aarch64-opc-2.c $(srcdir)/aarch64-tbl-2.h \
> $(srcdir)/i386-tbl.h $(srcdir)/i386-init.h $(srcdir)/i386-mnem.h \
> $(srcdir)/ia64-asmtab.c $(srcdir)/z8k-opc.h \
> $(srcdir)/msp430-decode.c \
> @@ -512,6 +512,14 @@ $(srcdir)/aarch64-dis-2.c: @MAINT@ aarch64-gen$(exeext_for_build)
> $(AM_V_GEN)./aarch64-gen$(exeext_for_build) --gen-dis > $@
> $(srcdir)/aarch64-opc-2.c: @MAINT@ aarch64-gen$(exeext_for_build)
> $(AM_V_GEN)./aarch64-gen$(exeext_for_build) --gen-opc > $@
> +$(srcdir)/aarch64-tbl-2.h: @MAINT@ aarch64-gen$(exeext_for_build)
> + $(AM_V_GEN)./aarch64-gen$(exeext_for_build) --gen-idx > $@
> +
> +aarch64-asm-2.lo: $(srcdir)/aarch64-tbl-2.h $(srcdir)/aarch64-asm-2.c
> +
> +aarch64-dis-2.lo: $(srcdir)/aarch64-tbl-2.h $(srcdir)/aarch64-dis-2.c
> +
> +aarch64-opc-2.lo: $(srcdir)/aarch64-tbl-2.h $(srcdir)/aarch64-opc-2.c
Is it necessary to specify the dependency of the .c files here? I would expect
it to be handled already by the default build rules.
>
> i386-gen$(EXEEXT_FOR_BUILD): i386-gen.o $(BUILD_LIB_DEPS)
> $(AM_V_CCLD)$(LINK_FOR_BUILD) i386-gen.o $(BUILD_LIBS)
> diff --git a/opcodes/aarch64-gen.c b/opcodes/aarch64-gen.c
> index 17fca2fe4d5..9cfa6f36da7 100644
> --- a/opcodes/aarch64-gen.c
> +++ b/opcodes/aarch64-gen.c
> @@ -23,6 +23,7 @@
> #include <stdio.h>
> #include <stdlib.h>
> #include <stdarg.h>
> +#include <ctype.h>
>
> #include "libiberty.h"
> #include "getopt.h"
> @@ -33,6 +34,10 @@
>
> static int debug = 0;
>
> +/* Table of labels for opcode nodes. We use this to stabilize the indices of
> + the data structures as the list of opcodes expands. */
> +const char **opcode_node_labels;
> +
> /* Structure used in the decoding tree to group a list of aarch64_opcode
> entries. */
>
> @@ -170,7 +175,8 @@ read_table (const struct aarch64_opcode* table)
> {
> fprintf (stderr,
> "%s (%08x,%08x): operands 1 and %u match, but tied=%u\n",
> - ent->name, ent->opcode, ent->mask, i + 1, ent->tied_operand);
> + ent->name, ent->opcode, ent->mask, i + 1,
> + ent->tied_operand);
> ++errors;
> }
> }
> @@ -199,7 +205,8 @@ read_table (const struct aarch64_opcode* table)
> /* If a subclass is set for one insn of an iclass, every insn of that
> iclass must have non-zero subclass field. */
> if ((iclass_has_subclasses_p[ent->iclass] && !(ent->flags & F_SUBCLASS))
> - || (!iclass_has_subclasses_p[ent->iclass] && (ent->flags & F_SUBCLASS)))
> + || (!iclass_has_subclasses_p[ent->iclass]
> + && (ent->flags & F_SUBCLASS)))
> {
> fprintf (stderr, "%s: unexpected subclass\n", ent->name);
> ++errors;
These two hunks appear to be irrelevant formatting changes, so shouldn't be
part of this patch.
> @@ -463,8 +470,8 @@ print_decision_tree_1 (unsigned int indent, struct bittree* bittree)
> indented_print (indent, " %s\n", pattern);
> indented_print (indent, " %s. */\n",
> get_aarch64_opcode (bittree->list)->name);
> - indented_print (indent, "return %u;\n",
> - real_index (bittree->list->index));
> + indented_print (indent, "return %s;\n",
> + opcode_node_labels[real_index (bittree->list->index)]);
> return;
> }
>
> @@ -492,7 +499,7 @@ print_decision_tree (struct bittree* bittree)
>
> printf ("/* Called by aarch64_opcode_lookup. */\n\n");
>
> - printf ("static int\n");
> + printf ("static enum aarch64_opcode_idx\n");
> printf ("aarch64_opcode_lookup_1 (uint32_t word)\n");
> printf ("{\n");
>
> @@ -529,16 +536,17 @@ print_find_next_opcode_1 (struct bittree* bittree)
> const aarch64_opcode *curr = get_aarch64_opcode (list);
> const aarch64_opcode *next = get_aarch64_opcode (list->next);
>
> - printf (" case %u: ",
> - (unsigned int)(curr - aarch64_opcode_table));
> + printf (" case %s:\n",
> + opcode_node_labels[curr - aarch64_opcode_table]);
> if (list->next != NULL)
> {
> - printf ("value = %u; break;\t", real_index (list->next->index));
> + printf (" value = %s;\n break;\t",
> + opcode_node_labels[real_index (list->next->index)]);
> printf ("/* %s --> %s. */\n", curr->name, next->name);
> }
> else
> {
> - printf ("return NULL;\t\t");
> + printf (" return NULL;\t\t");
> printf ("/* %s --> NULL. */\n", curr->name);
> }
>
> @@ -566,8 +574,8 @@ print_find_next_opcode (struct bittree* bittree)
> printf ("aarch64_find_next_opcode (const aarch64_opcode *opcode)\n");
> printf ("{\n");
> printf (" /* Use the index as the key to locate the next opcode. */\n");
> - printf (" int key = opcode - aarch64_opcode_table;\n");
> - printf (" int value;\n");
> + printf (" enum aarch64_opcode_idx key = opcode - aarch64_opcode_table;\n");
> + printf (" enum aarch64_opcode_idx value;\n");
> printf (" switch (key)\n");
> printf (" {\n");
>
> @@ -630,8 +638,8 @@ print_find_real_opcode (const opcode_node *table, int num)
> printf ("aarch64_find_real_opcode (const aarch64_opcode *opcode)\n");
> printf ("{\n");
> printf (" /* Use the index as the key to locate the real opcode. */\n");
> - printf (" int key = opcode - aarch64_opcode_table;\n");
> - printf (" int value;\n");
> + printf (" enum aarch64_opcode_idx key = opcode - aarch64_opcode_table;\n");
> + printf (" enum aarch64_opcode_idx value;\n");
> printf (" switch (key)\n");
> printf (" {\n");
>
> @@ -640,9 +648,11 @@ print_find_real_opcode (const opcode_node *table, int num)
> const opcode_node *real = table + i;
> const opcode_node *alias = real->next;
> for (; alias; alias = alias->next)
> - printf (" case %u:\t/* %s */\n", real_index (alias->index),
> + printf (" case %s:\t/* %s */\n",
> + opcode_node_labels[real_index (alias->index)],
> get_aarch64_opcode (alias)->name);
> - printf (" value = %u;\t/* --> %s. */\n", real_index (real->index),
> + printf (" value = %s;\t/* --> %s. */\n",
> + opcode_node_labels[real_index (real->index)],
> get_aarch64_opcode (real)->name);
> printf (" break;\n");
> }
> @@ -671,8 +681,8 @@ print_find_alias_opcode (const opcode_node *table, int num)
> printf ("aarch64_find_alias_opcode (const aarch64_opcode *opcode)\n");
> printf ("{\n");
> printf (" /* Use the index as the key to locate the alias opcode. */\n");
> - printf (" int key = opcode - aarch64_opcode_table;\n");
> - printf (" int value;\n");
> + printf (" enum aarch64_opcode_idx key = opcode - aarch64_opcode_table;\n");
> + printf (" enum aarch64_opcode_idx value;\n");
> printf (" switch (key)\n");
> printf (" {\n");
>
> @@ -680,8 +690,9 @@ print_find_alias_opcode (const opcode_node *table, int num)
> {
> const opcode_node *node = table + i;
> assert (node->next);
> - printf (" case %u: value = %u; break;", real_index (node->index),
> - real_index (node->next->index));
> + printf (" case %s:\n value = %s;\n break;",
> + opcode_node_labels[real_index (node->index)],
> + opcode_node_labels[real_index (node->next->index)]);
> printf ("\t/* %s --> %s. */\n", get_aarch64_opcode (node)->name,
> get_aarch64_opcode (node->next)->name);
> }
> @@ -710,8 +721,8 @@ print_find_next_alias_opcode (const opcode_node *table, int num)
> printf ("aarch64_find_next_alias_opcode (const aarch64_opcode *opcode)\n");
> printf ("{\n");
> printf (" /* Use the index as the key to locate the next opcode. */\n");
> - printf (" int key = opcode - aarch64_opcode_table;\n");
> - printf (" int value;\n");
> + printf (" enum aarch64_opcode_idx key = opcode - aarch64_opcode_table;\n");
> + printf (" enum aarch64_opcode_idx value;\n");
> printf (" switch (key)\n");
> printf (" {\n");
>
> @@ -723,8 +734,9 @@ print_find_next_alias_opcode (const opcode_node *table, int num)
> continue;
> while (node->next->next)
> {
> - printf (" case %u: value = %u; break;", real_index (node->next->index),
> - real_index (node->next->next->index));
> + printf (" case %s:\n value = %s;\n break;",
> + opcode_node_labels[real_index (node->next->index)],
> + opcode_node_labels[real_index (node->next->next->index)]);
> printf ("\t/* %s --> %s. */\n",
> get_aarch64_opcode (node->next)->name,
> get_aarch64_opcode (node->next->next)->name);
> @@ -1142,6 +1154,89 @@ print_operand_extractor (void)
> printf (" }\n");
> printf ("}\n");
> }
> +
> +/* Generate a set of labels for the opcode table, so that we can refer to index
> + entries through stable names. The general format for the label is:
> + <opcode>_<mnemonic>(_operand_name)*
> + We currently do not need to use the qualifiers to discriminate further, and
> + hopefully never will. */
> +#define ENUM_PFX "aa64_opid_"
All the existing enums we have that use prefixed names used AARCH64_[CAPS]_ as
the prefix. I don't see any reason to do differently here - i.e.
"AARCH64_OPID_" would be more consistent. Also, with the below suggestions we
only need one reference to this, so we could just inline the #define.
> +#define MAX_LABEL_LEN 100
> +void
> +label_opcode_nodes (void)
> +{
> + unsigned count = sizeof (aarch64_opcode_table) / sizeof (aarch64_opcode);
> + unsigned i;
> + const aarch64_opcode *opc;
> +
> + opcode_node_labels = malloc (count * sizeof (char *));
> +
> + for (i = 0, opc = aarch64_opcode_table; i < count - 1; ++i, ++opc)
> + {
> + char buf[MAX_LABEL_LEN];
> + char *p;
> + unsigned j;
> + size_t l;
> +
> + strcpy (buf, ENUM_PFX);
> + p = buf + sizeof(ENUM_PFX) - 1;
> +
> + assert (p - buf + 9 < MAX_LABEL_LEN);
> + sprintf (p, "%08x_", opc->opcode);
> + p += 9;
> + l = strlen (opc->name);
> + assert (p - buf + l < MAX_LABEL_LEN);
> + strcpy (p, opc->name);
> + p += l;
> + for (j = 0;
> + j < AARCH64_MAX_OPND_NUM && opc->operands[j] != AARCH64_OPND_NIL;
> + j++)
> + {
> + l = strlen (operands[opc->operands[j]].str);
> + assert (p - buf + 1 + l < MAX_LABEL_LEN);
> + *(p++) = '_';
> + strcpy (p, operands[opc->operands[j]].str);
> + p += l;
> + }
This feels quite verbose and repetitive with all the separate asserts and
pointer increments. How about replacing the above with:
char buf[MAX_LABEL_LEN];
char *p = buf;
p += snprintf (p, "AARCH64_OPID_%08x_%s", MAX_LABEL_LEN, opc->opcode,
opc->name);
assert (p < buf + MAX_LABEL_LEN);
for (...)
{
p + snprintf (p, "_%s", buf + MAX_LABEL_LEN - p, operands[opc->operands[j]].str);
assert (p < buf + MAX_LABEL_LEN);
}
> +
> + /* Sanitize the opcode name into something that can be used as
> + an identifier. */
> + for (p = buf + sizeof (ENUM_PFX) + 9; *p != '\0'; ++p)
It would be clearer to just sanitise the whole string without skipping past the
initial segment.
> + if (!(isalpha (*p) || isdigit (*p) || *p == '_'))
> + *p = '_';
> +
> + opcode_node_labels[i] = strdup (buf);
> + }
> +
> + opcode_node_labels[i] = ENUM_PFX "max";
We can write this explicitly as "AARCH64_OPID_MAX" and drop the #define.
> +}
> +
> +/* Emit the opcode labels as an enum */
> +void
> +print_opcode_labels_defn (void)
> +{
> + unsigned count = sizeof (aarch64_opcode_table) / sizeof (aarch64_opcode);
> + unsigned i;
> +
> + printf ("/* Mnemonic names for the indices in the aarch64_opcode table,\n"
> + " so that we can refer to the entries by name. This serves two\n"
> + " main purposes. Firstly, the names change rarely, if at all,\n"
> + " so diffs in the generated files as entries are added to the\n"
> + " table are much smaller. Secondly, the names are useful when\n"
> + " viewing the generated code in a debugger, giving a better\n"
You can drop "in a debugger" - it's useful outside a debugger as well.
> + " indication of what is going on.\n\n");
> + printf (" The format of the names is:\n"
> + "\taa64_opid_<opcode>_<mnemonic>(_<operand_type>)*\n"
> + " We do not include the operand modifier information in the names\n"
> + " as this would make them too long and is currently unnecessary\n"
> + " for generating a unique name. */\n\n");
> +
> + printf ("enum aarch64_opcode_idx\n{\n");
> + for (i = 0; i < count; ++i)
> + printf (" %s,\n", opcode_node_labels[i]);
> + printf ("};\n");
> +}
> +
>
> /* Table indexed by opcode enumerator stores the index of the corresponding
> opcode entry in aarch64_opcode_table. */
> @@ -1185,18 +1280,19 @@ print_get_opcode (void)
>
> /* Print the table. */
> printf ("\n");
> - printf ("/* Indexed by an enum aarch64_op enumerator, the value is the offset of\n\
> - the corresponding aarch64_opcode entry in the aarch64_opcode_table. */\n\n");
> - printf ("static const unsigned op_enum_table [] =\n");
> + printf ("/* Indexed by an enum aarch64_op enumerator, the value is the\n"
> + " offset of the corresponding aarch64_opcode entry in the\n"
> + " aarch64_opcode_table. */\n\n");
> + printf ("static const enum aarch64_opcode_idx op_enum_table [] =\n");
> printf ("{\n");
> for (i = 0; i < num; ++i)
> - printf (" %u,\n", op_enum_table[i]);
> + printf (" %s,\n", opcode_node_labels[op_enum_table[i]]);
> printf ("};\n");
>
> /* Print the function. */
> printf ("\n");
> - printf ("/* Given the opcode enumerator OP, return the pointer to the corresponding\n");
> - printf (" opcode entry. */\n");
> + printf ("/* Given the opcode enumerator OP, return the pointer to the\n"
> + " corresponding opcode entry. */\n");
These two comments haven't changed, so the reformatting probably shouldn't be
part of this patch.
> printf ("\n");
> printf ("const aarch64_opcode *\n");
> printf ("aarch64_get_opcode (enum aarch64_op op)\n");
> @@ -1228,6 +1324,7 @@ struct option long_options[] =
> {"gen-opc", no_argument, NULL, 'c'},
> {"gen-asm", no_argument, NULL, 'a'},
> {"gen-dis", no_argument, NULL, 's'},
> + {"gen-idx", no_argument, NULL, 'i'},
> {0, no_argument, NULL, 0}
> };
>
> @@ -1243,7 +1340,9 @@ usage (FILE * stream, int status)
> {
> fprintf (stream, "Usage: %s [-V | --version] [-d | --debug] [--help]\n",
> program_name);
> - fprintf (stream, "\t[ [-c | --gen-opc] | [-a | --gen-asm] | [-s | --gen-dis] ]\n");
> + fprintf (stream,
> + "\t[ [-c | --gen-opc] | [-a | --gen-asm] "
> + "| [-s | --gen-dis] | [ --gen-idx | -i ]]\n");
> xexit (status);
> }
>
> @@ -1255,6 +1354,7 @@ main (int argc, char **argv)
> int gen_opcode_p = 0;
> int gen_assembler_p = 0;
> int gen_disassembler_p = 0;
> + int gen_indices_p = 0;
>
> program_name = *argv;
> xmalloc_set_program_name (program_name);
> @@ -1273,6 +1373,9 @@ main (int argc, char **argv)
> case '?':
> usage (stderr, 0);
> break;
> + case 'i':
> + gen_indices_p = 1;
> + break;
> case 'c':
> gen_opcode_p = 1;
> break;
> @@ -1290,10 +1393,11 @@ main (int argc, char **argv)
> if (argc == 1 || optind != argc)
> usage (stdout, 1);
>
> - if (gen_opcode_p + gen_assembler_p + gen_disassembler_p > 1)
> + if (gen_opcode_p + gen_assembler_p + gen_disassembler_p + gen_indices_p > 1)
> {
> - printf ("Please specify only one of the following options\n\
> - [-c | --gen-opc] [-a | --gen-asm] [-s | --gen-dis]\n");
> + printf ("Please specify only one of the following options\n\t"
> + "[-c | --gen-opc] [-a | --gen-asm] "
> + "[-s | --gen-dis] [-i | --gen-idx]\n");
> xexit (2);
> }
>
> @@ -1303,7 +1407,10 @@ main (int argc, char **argv)
> if (debug)
> print_divide_result (decoder_tree);
>
> - printf ("/* This file is automatically generated by aarch64-gen. Do not edit! */\n");
> + label_opcode_nodes ();
> +
> + printf ("/* This file is automatically generated by aarch64-gen. "
> + "Do not edit! */\n");
> printf ("/* Copyright (C) 2012-2025 Free Software Foundation, Inc.\n\
> Contributed by ARM Ltd.\n\
> \n\
> @@ -1324,14 +1431,20 @@ main (int argc, char **argv)
> see <http://www.gnu.org/licenses/>. */\n");
>
> printf ("\n");
> - printf ("#include \"sysdep.h\"\n");
> - if (gen_opcode_p)
> - printf ("#include \"aarch64-opc.h\"\n");
> - if (gen_assembler_p)
> - printf ("#include \"aarch64-asm.h\"\n");
> - if (gen_disassembler_p)
> - printf ("#include \"aarch64-dis.h\"\n");
> - printf ("\n");
> + if (gen_indices_p)
> + print_opcode_labels_defn ();
> + else
> + {
> + printf ("#include \"sysdep.h\"\n");
> + printf ("#include \"aarch64-tbl-2.h\"\n");
> + if (gen_opcode_p)
> + printf ("#include \"aarch64-opc.h\"\n");
> + if (gen_assembler_p)
> + printf ("#include \"aarch64-asm.h\"\n");
> + if (gen_disassembler_p)
> + printf ("#include \"aarch64-dis.h\"\n");
> + printf ("\n");
> + }
>
> /* Generate opcode entry lookup for the disassembler. */
> if (gen_disassembler_p)
> --
> 2.43.0
>
More information about the Binutils
mailing list