On Thu, May 28, 2015 at 11:31:40AM +0100, Neil Roberts wrote:
> Ben Widawsky <benjamin.widaw...@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 <matts...@gmail.com>
> > Cc: Neil Roberts <n...@linux.intel.com>
> > Cc: Mark Janes <mark.a.ja...@intel.com>
> > Signed-off-by: Ben Widawsky <b...@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...
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
