[PATCH 5/6] aarch64: Use an enum to refer to indices in the opcode table

[Richard Earnshaw (lists)]

On 18/07/2025 16:34, Alice Carlotti wrote:
> On Fri, Jul 18, 2025 at 11:04:00AM +0100, Richard Earnshaw wrote:
>>  	$(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.
> 

In the strictest sense, probably not.  But it seems strange to mention the dependency on aarch64-tbl-2.h and omit the dependency on the main source file itself.  It's pretty harmless, so I've left it in.


>> @@ -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.

Moved to another patch earlier in the series.


>> @@ -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.

I've changed it to A64_OPID_ and then embedded it.  These enumeration values have very long names (some are around 70 chars), so to keep things a little more manageable I've not used the additional 'ARCH' letters.  Technically, A64 is the instruction set used in AArch64, so it's not even wrong :)

> 
>> +#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);
> 	}
> 

Fixed, but I've added some code to check for snprintf returning negative values as well.

>> +
>> +      /* 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.

Agreed, fixed.

> 
>> +	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.
> 

Fixed, but see above.

>> +}
>> +
>> +/* 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.
> 

I changed it to 'for example, in a debugger'.  It was never meant to mean that was the only place where this might be useful.



>> @@ -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.

Moved to the new cleanup patch.

R.