[Mesa-dev] [PATCH] i965: Replace illegal compacted NOP with valid compact instruction

[Matt Turner]

On Wed, Jul 8, 2015 at 10:58 PM, Zhenyu Wang <zhenyuw at linux.intel.com> wrote:
> NOP actually has no compact version, but we use it for instruction
> alignment for compact kernel. Although it seems working on HW, it is
> illegal and might not be valid for any future one.
>
> This trys to get a temporary compact instruction with no effect for
> alignment to replace compacted NOP. G45 spec has note that HW compact
> logic could determine NENOP and drop it right away, so we can still
> keep with that.
>
> v2: rebase to master, we still need this to work with internal tool.
>
> Signed-off-by: Zhenyu Wang <zhenyuw at linux.intel.com>
> ---
>  src/mesa/drivers/dri/i965/brw_eu_compact.c | 41 +++++++++++++++++++++++++-----
>  1 file changed, 35 insertions(+), 6 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_eu_compact.c b/src/mesa/drivers/dri/i965/brw_eu_compact.c
> index 67f0b45..719667a 100644
> --- a/src/mesa/drivers/dri/i965/brw_eu_compact.c
> +++ b/src/mesa/drivers/dri/i965/brw_eu_compact.c
> @@ -1367,6 +1367,39 @@ brw_init_compaction_tables(const struct brw_device_info *devinfo)
>     }
>  }
>
> +static void
> +brw_get_noop_compact(struct brw_codegen *p, brw_compact_inst *dst)

I'd rather call this function fill_compaction_padding() or something
similar -- there's no need for the brw_ prefix since it's static, it's
not "get"ting anything, and "noop" in the name is a little confusing
since it's not emitting a NOP. :)

> +{
> +   const struct brw_device_info *devinfo = p->devinfo;
> +   brw_inst *inst, i;
> +   struct brw_reg g0 = brw_vec8_grf(0, 0);
> +
> +   memset(dst, 0, sizeof(*dst));
> +
> +   /* G45 compact logic could recognize NENOP and drop right away. */
> +   if (devinfo->is_g4x) {
> +      brw_compact_inst_set_opcode(dst, BRW_OPCODE_NENOP);
> +      brw_compact_inst_set_cmpt_control(dst, true);
> +      return;
> +   }
> +
> +   /*
> +    * As NOP has no legal compact version, try to use a legal compact
> +    * instruction for compact instruction alignment.
> +    */
> +   brw_push_insn_state(p);
> +   brw_set_default_predicate_control(p, BRW_PREDICATE_NONE);
> +   brw_set_default_access_mode(p, BRW_ALIGN_1);
> +   inst = brw_MOV(p, g0, g0);
> +   memcpy(&i, inst, sizeof(brw_inst));
> +   brw_pop_insn_state(p);
> +
> +   if (!brw_try_compact_instruction(devinfo, dst, &i)) {
> +      fprintf(stderr, "Failed to generate compact inst for alignment!\n");
> +      exit(1);

This isn't an error we ever expect a user to hit, to let's make it an assert:

bool UNUSED ret = brw_try_compact_instruction(devinfo, dst, &i);
assert(ret);

With those small changes, this patch is

Reviewed-by: Matt Turner <mattst88 at gmail.com>

I'd be happy to make the changes myself and commit the patch if you'd
like -- just tell me so. :)

Thanks Zhenyu!