[PATCH,V5 7/9] opcodes: aarch64: enforce checks on subclass flags in aarch64-gen.c

[Richard Sandiford]

Indu Bhagat <indu.bhagat@oracle.com> writes:
> [No changes in V5]
>
> [New in V4]
>
> Enforce some checks on the newly added subclass flags:
>   - If a subclass is set of one insn of an iclass, every insn of that
>     iclass must have non-zero subclass field.
>   - For all other iclasses, the subclass bits are zero for all insns.
>
> include/
>         * opcode/aarch64.h (enum aarch64_insn_class): Identify the
> 	maximum iclass enum value.
>
> opcodes/
>         * aarch64-gen.c (iclass_has_subclasses_p): New array of bool.
>         (read_table): Enforce checks on subclass flags.
> ---
>  include/opcode/aarch64.h |  3 ++-
>  opcodes/aarch64-gen.c    | 19 +++++++++++++++++++
>  2 files changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/include/opcode/aarch64.h b/include/opcode/aarch64.h
> index cf74514f710..77ee86acc77 100644
> --- a/include/opcode/aarch64.h
> +++ b/include/opcode/aarch64.h
> @@ -1108,7 +1108,8 @@ enum aarch64_insn_class
>    sve2_urqvs,
>    sve_index1,
>    rcpc3,
> -  lut
> +  lut,
> +  last_iclass = lut
>  };
>  
>  /* Opcode enumerators.  */
> diff --git a/opcodes/aarch64-gen.c b/opcodes/aarch64-gen.c
> index 02dcde1f676..2473f6704ce 100644
> --- a/opcodes/aarch64-gen.c
> +++ b/opcodes/aarch64-gen.c
> @@ -123,6 +123,8 @@ get_aarch64_opcode (const opcode_node *opcode_node)
>    return &index2table (opcode_node->index)[real_index (opcode_node->index)];
>  }
>  
> +static bool iclass_has_subclasses_p[last_iclass];
> +
>  static void
>  read_table (const struct aarch64_opcode* table)
>  {
> @@ -181,6 +183,9 @@ read_table (const struct aarch64_opcode* table)
>  	  ++errors;
>  	}
>  
> +      if (ent->flags & F_SUBCLASS)
> +	iclass_has_subclasses_p[ent->iclass] = true;
> +
>        *new_ent = new_opcode_node ();
>        (*new_ent)->opcode = ent->opcode;
>        (*new_ent)->mask = ent->mask;

I was thinking instead of setting iclass_has_subclasses_p to true for
each class that patch 8 expects to have a subclass (i.e. a hard-coded list).
I agree that checking for self-consistency is good enough though.

> @@ -188,6 +193,20 @@ read_table (const struct aarch64_opcode* table)
>        new_ent = &((*new_ent)->next);
>      } while ((++ent)->name);
>  
> +  ent = table;
> +  do
> +    {
> +      /* 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)))

I suppose the above makes the line above redundant, but I agree it's
clearer to keep it.

> +	{
> +	    fprintf (stderr, "%s: unexpected subclass\n", ent->name);
> +	    ++errors;

Formatting nit: body is indented two spaces too far.

OK with that trivial change, thanks.

Richard

> +	}
> +      ent++;
> +    } while (ent->name);
> +
>    if (errors)
>      {
>        fprintf (stderr, "%u errors, exiting\n", errors);