[Mesa-dev] [PATCH 2/2] intel/compiler: Use null destination register for memory fence messages

[Anuj Phogat]

On Wed, Feb 7, 2018 at 12:24 PM, Francisco Jerez <currojerez at riseup.net>
wrote:

> Anuj Phogat <anuj.phogat at gmail.com> writes:
>
> > On Wed, Feb 7, 2018 at 11:55 AM, Francisco Jerez <currojerez at riseup.net>
> > wrote:
> >
> >> Anuj Phogat <anuj.phogat at gmail.com> writes:
> >>
> >> > On Wed, Feb 7, 2018 at 11:14 AM, Kenneth Graunke <
> kenneth at whitecape.org>
> >> > wrote:
> >> >
> >> >> On Tuesday, February 6, 2018 5:09:10 PM PST Anuj Phogat wrote:
> >> >> > From Message Descriptor section in gfxspecs:
> >> >> >  "Memory fence messages without Commit Enable set do not return
> >> >> >   anything to the thread (response length is 0 and destination
> >> >> >   register is null)."
> >> >> >
> >> >> > It fixes a piglit GPU hang in simulation environment.
> >> >> > Piglit test: arb_shader_image_load_store-shader-mem-barrier
> >> >> >
> >> >> > Signed-off-by: Anuj Phogat <anuj.phogat at gmail.com>
> >> >> > Cc: mesa-stable at lists.freedesktop.org
> >> >> > ---
> >> >> >  src/intel/compiler/brw_eu_emit.c | 8 +++++++-
> >> >> >  1 file changed, 7 insertions(+), 1 deletion(-)
> >> >> >
> >> >> > diff --git a/src/intel/compiler/brw_eu_emit.c
> >> >> b/src/intel/compiler/brw_eu_emit.c
> >> >> > index 1fb9aab51c..c66b813af8 100644
> >> >> > --- a/src/intel/compiler/brw_eu_emit.c
> >> >> > +++ b/src/intel/compiler/brw_eu_emit.c
> >> >> > @@ -3290,7 +3290,13 @@ brw_memory_fence(struct brw_codegen *p,
> >> >> >      */
> >> >> >     insn = next_insn(p, BRW_OPCODE_SEND);
> >> >> >     dst = retype(dst, BRW_REGISTER_TYPE_UW);
> >> >> > -   brw_set_dest(p, insn, dst);
> >> >> > +
> >> >> > +  /* From Message Descriptor section in gfxspecs:
> >> >> > +        "Memory fence messages without Commit Enable set do not
> >> return
> >> >> > +         anything to the thread (response length is 0 and
> destination
> >> >> > +         register is null).
> >> >> > +   */
> >> >> > +   brw_set_dest(p, insn, retype(vec1(brw_null_reg()),
> >> >> BRW_REGISTER_TYPE_UW));
> >> >> >     brw_set_src0(p, insn, dst);
> >> >> >     brw_set_memory_fence_message(p, insn,
> >> GEN7_SFID_DATAPORT_DATA_CACHE,
> >> >> >                                  commit_enable);
> >> >> >
> >> >>
> >> >> This seems wrong - it looks like you're doing this for messages with
> >> >> commit enable set, which do return things...
> >> >>
> >> >> ​Right. I've to check for commit_enable to use null or non-null
> >> destination
> >> > here. I'll send out a v2.
> >> > ​
> >> >
> >>
> >> You can also just drop the patch, unless you want to change the
> >> front-end in addition to stop allocating a destination for memory fences
> >> on HSW-SKL in order to save a small amount of register pressure?
> >>
> > ​It also fixes a fulsim error other than reducing register pressure.
>
> On what platform?
>
​I noticed this on Icelake. But, I'm sure it exists on CNL as well.​


>
> > I'll look in to how to stop allocating dst register for memory fences
> > and send out a v2. Thanks for the suggestion.  ​
> >
>
> In that case I think it would be nice to avoid duplicating the logic
> used to decide whether to enable the commit bit or not.  You can
> allocate the register conditionally in the visitor (providing a null
> register on HSW-SKL), and have brw_memory_fence() enable the commit bit
> if and only if dst is not null, that way we have the guarantee that the
> back-end and front-end are kept in sync.
> ​sounds good to me.​
>
>

> >
> >
> >>
> >> >
> >> >> Cc'ing Curro.
> >> >>
> >>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180207/b1a750b1/attachment-0001.html>