[Mesa-dev] [PATCH 5/7] i965/disasm: Mark instructions that no longer exist in the opcode_desc tables.

[Iago Toral]

On Thu, 2016-04-28 at 00:19 -0700, Francisco Jerez wrote:
> With this small addition we can now easily determine on which
> generations a given instruction is supported from the opcode_desc
> tables alone.
> ---
>  src/mesa/drivers/dri/i965/brw_disasm.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/src/mesa/drivers/dri/i965/brw_disasm.c b/src/mesa/drivers/dri/i965/brw_disasm.c
> index 0125434..5c6f3e2 100644
> --- a/src/mesa/drivers/dri/i965/brw_disasm.c
> +++ b/src/mesa/drivers/dri/i965/brw_disasm.c
> @@ -30,6 +30,14 @@
>  #include "brw_inst.h"
>  #include "brw_eu.h"
>  
> +/**
> + * Special opcode_desc entry that marks the instruction as no longer existing.
> + * Unless explicitly specified using this marker a hardware generation is
> + * assumed to have inherited all opcodes defined and not removed by previous
> + * generations.
> + */
> +#define REMOVED { .name = "***removed***", .nsrc = ~0, .ndst = ~0 }
> +
>  static const struct opcode_desc gen4_opcode_descs[128] = {
>     [BRW_OPCODE_MOV]      = { .name = "mov",     .nsrc = 1, .ndst = 1 },
>     [BRW_OPCODE_FRC]      = { .name = "frc",     .nsrc = 1, .ndst = 1 },
> @@ -94,6 +102,8 @@ static const struct opcode_desc gen6_opcode_descs[128] = {
>     [BRW_OPCODE_MATH]     = { .name = "math",    .nsrc = 2, .ndst = 1 },
>     [BRW_OPCODE_MAD]      = { .name = "mad",     .nsrc = 3, .ndst = 1 },
>     [BRW_OPCODE_LRP]      = { .name = "lrp",     .nsrc = 3, .ndst = 1 },
> +   [BRW_OPCODE_IFF]      = REMOVED,
> +   [BRW_OPCODE_DO]       = REMOVED,
>  };
>  
>  static const struct opcode_desc gen7_opcode_descs[128] = {
> @@ -111,6 +121,8 @@ static const struct opcode_desc gen7_opcode_descs[128] = {
>  };
>  
>  static const struct opcode_desc gen8_opcode_descs[128] = {
> +   [BRW_OPCODE_F32TO16]  = REMOVED,
> +   [BRW_OPCODE_F16TO32]  = REMOVED,
>     [BRW_OPCODE_CSEL]     = { .name = "csel",    .nsrc = 3, .ndst = 1 },
>  };
>  
> @@ -151,7 +163,6 @@ opcode_desc_table_for(const struct brw_device_info *devinfo, enum opcode opcode)
>  
>  /* Return the matching opcode_desc for the specified opcode number and
>   * hardware generation, or NULL if the opcode is not supported by the device.
> - * XXX -- Actually check whether the opcode is supported.
>   */
>  const struct opcode_desc *
>  brw_opcode_desc(const struct brw_device_info *devinfo, enum opcode opcode)
> @@ -159,7 +170,8 @@ brw_opcode_desc(const struct brw_device_info *devinfo, enum opcode opcode)
>     const struct opcode_desc *opcode_descs =
>        opcode_desc_table_for(devinfo, opcode);
>  
> -   if (opcode_descs)
> +   if (opcode_descs && (opcode_descs[opcode].nsrc != ~0 &&
> +                        opcode_descs[opcode].ndst != ~0))

This is a nitpick so feel free to ignore it if you want:

Since you encode special removed entries in a macro I think it would be
nicer if the check for whether an entry has been removed is also done
through a similar macro so callers don't need to know how these special
entries are encoded:

#define IS_REMOVED(entry) (entry.nsrc != ~0 && entry.ndst != ~0)

>        return &opcode_descs[opcode];
>     else
>        return NULL;