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

[Ben Widawsky]

On Thu, May 28, 2015 at 11:31:40AM +0100, Neil Roberts wrote:
> 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?

Well eot just means bit 127, and in certain circumstances outside of send, I
believe it's valid to have the bit set (the src1 imm being the one example I can
find, if you are smart about it). We can defer to Matt since he said he'd look a
bit today.

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

I didn't see that patch, this was the patch that Mark originally ran through
piglit as well (http://otc-mesa-ci.jf.intel.com/job/Leeroy/99272/). I'm not
opposed to this simpler solution if Matt determines it's equally viable and or
superior in some way. To me it seemed a bit more readable and accurate to only
check EOT when it was actually an EOT (the sends).

> 
> 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?
> 

Yep. I have no clue on that one.

> 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:

Apologies for not realizing you wrote the same patch...