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

[Francisco Jerez]

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'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.

>
>
>>
>> >
>> >> Cc'ing Curro.
>> >>
>>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 227 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/mesa-stable/attachments/20180207/3c55e8e1/attachment.sig>