[Mesa-dev] [PATCH] i965: Disable compaction for EOT send messages

[Neil Roberts]

Ben Widawsky <benjamin.widawsky at intel.com> writes:

> AFAICT, there is no real way to make sure a send message with EOT is
> properly ignored from compact, nor can I see a way to actually encode
> EOT while compacting. Before the single send optimization we'd always
> bail because we hit the is_immediate && !is_compactable_immediate
> case. However, with single send, is_immediate is not true, and so we
> end up trying to compact the un-compactible.
>
> Without this, any compacting single send instruction will hang because
> the EOT isn't there. I am not sure how I didn't hit this when I
> originally enabled the optimization. I didn't check if some
> surrounding code changed.
>
> NOTE: This needs another piglit run or two before merge.
>
> I know Neil and Matt were both looking into this. I did a quick search
> and didn't see any patches out there to handle this. Please ignore if
> this has already been sent by someone. (Direct me to it and I will
> review it).
>
> Cc: Matt Turner <mattst88 at gmail.com>
> Cc: Neil Roberts <neil at linux.intel.com>
> Cc: Mark Janes <mark.a.janes at intel.com>
> Signed-off-by: Ben Widawsky <ben at bwidawsk.net>
> ---
>  src/mesa/drivers/dri/i965/brw_eu_compact.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_eu_compact.c b/src/mesa/drivers/dri/i965/brw_eu_compact.c
> index 69cb114..67f0b45 100644
> --- a/src/mesa/drivers/dri/i965/brw_eu_compact.c
> +++ b/src/mesa/drivers/dri/i965/brw_eu_compact.c
> @@ -849,6 +849,12 @@ set_3src_source_index(const struct brw_device_info *devinfo,
>  static bool
>  has_unmapped_bits(const struct brw_device_info *devinfo, brw_inst *src)
>  {
> +   /* EOT can only be mapped on a send if the src1 is an immediate */

Can we really map EOT if the src1 is immediate?

> +   if ((brw_inst_opcode(devinfo, src) == BRW_OPCODE_SENDC ||
> +        brw_inst_opcode(devinfo, src) == BRW_OPCODE_SEND) &&

Is there any reason to limit this to send and sendc? If there's no way
to map EOT why not just to if (brw_inst_eot(...)) return true?

For what it's worth, I ran my original patch¹ through shader-db and it
didn't make any difference, which is good.

Do we not also need to fix the problem with the destination register
being used as a temporary? This was mentioned by Matt on IRC. Maybe he
is looking into it?

Regards,
- Neil

1. https://github.com/bpeel/mesa/commit/6abda467fa3ad7060127

> +       brw_inst_eot(devinfo, src))
> +      return true;
> +
>     /* Check for instruction bits that don't map to any of the fields of the
>      * compacted instruction.  The instruction cannot be compacted if any of
>      * them are set.  They overlap with:
> -- 
> 2.4.1