<div dir="ltr"><div class="gmail_default" style="font-family:verdana,sans-serif;font-size:small"><br></div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Feb 7, 2018 at 12:24 PM, Francisco Jerez <span dir="ltr"><<a href="mailto:currojerez@riseup.net" target="_blank">currojerez@riseup.net</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">Anuj Phogat <<a href="mailto:anuj.phogat@gmail.com">anuj.phogat@gmail.com</a>> writes:<br>
<br>
> On Wed, Feb 7, 2018 at 11:55 AM, Francisco Jerez <<a href="mailto:currojerez@riseup.net">currojerez@riseup.net</a>><br>
> wrote:<br>
><br>
>> Anuj Phogat <<a href="mailto:anuj.phogat@gmail.com">anuj.phogat@gmail.com</a>> writes:<br>
>><br>
>> > On Wed, Feb 7, 2018 at 11:14 AM, Kenneth Graunke <<a href="mailto:kenneth@whitecape.org">kenneth@whitecape.org</a>><br>
>> > wrote:<br>
>> ><br>
>> >> On Tuesday, February 6, 2018 5:09:10 PM PST Anuj Phogat wrote:<br>
>> >> > From Message Descriptor section in gfxspecs:<br>
>> >> >  "Memory fence messages without Commit Enable set do not return<br>
>> >> >   anything to the thread (response length is 0 and destination<br>
>> >> >   register is null)."<br>
>> >> ><br>
>> >> > It fixes a piglit GPU hang in simulation environment.<br>
>> >> > Piglit test: arb_shader_image_load_store-<wbr>shader-mem-barrier<br>
>> >> ><br>
>> >> > Signed-off-by: Anuj Phogat <<a href="mailto:anuj.phogat@gmail.com">anuj.phogat@gmail.com</a>><br>
>> >> > Cc: <a href="mailto:mesa-stable@lists.freedesktop.org">mesa-stable@lists.freedesktop.<wbr>org</a><br>
>> >> > ---<br>
>> >> >  src/intel/compiler/brw_eu_<wbr>emit.c | 8 +++++++-<br>
>> >> >  1 file changed, 7 insertions(+), 1 deletion(-)<br>
>> >> ><br>
>> >> > diff --git a/src/intel/compiler/brw_eu_<wbr>emit.c<br>
>> >> b/src/intel/compiler/brw_eu_<wbr>emit.c<br>
>> >> > index 1fb9aab51c..c66b813af8 100644<br>
>> >> > --- a/src/intel/compiler/brw_eu_<wbr>emit.c<br>
>> >> > +++ b/src/intel/compiler/brw_eu_<wbr>emit.c<br>
>> >> > @@ -3290,7 +3290,13 @@ brw_memory_fence(struct brw_codegen *p,<br>
>> >> >      */<br>
>> >> >     insn = next_insn(p, BRW_OPCODE_SEND);<br>
>> >> >     dst = retype(dst, BRW_REGISTER_TYPE_UW);<br>
>> >> > -   brw_set_dest(p, insn, dst);<br>
>> >> > +<br>
>> >> > +  /* From Message Descriptor section in gfxspecs:<br>
>> >> > +        "Memory fence messages without Commit Enable set do not<br>
>> return<br>
>> >> > +         anything to the thread (response length is 0 and destination<br>
>> >> > +         register is null).<br>
>> >> > +   */<br>
>> >> > +   brw_set_dest(p, insn, retype(vec1(brw_null_reg()),<br>
>> >> BRW_REGISTER_TYPE_UW));<br>
>> >> >     brw_set_src0(p, insn, dst);<br>
>> >> >     brw_set_memory_fence_message(<wbr>p, insn,<br>
>> GEN7_SFID_DATAPORT_DATA_CACHE,<br>
>> >> >                                  commit_enable);<br>
>> >> ><br>
>> >><br>
>> >> This seems wrong - it looks like you're doing this for messages with<br>
>> >> commit enable set, which do return things...<br>
>> >><br>
>> >> ​Right. I've to check for commit_enable to use null or non-null<br>
>> destination<br>
>> > here. I'll send out a v2.<br>
>> > ​<br>
>> ><br>
>><br>
>> You can also just drop the patch, unless you want to change the<br>
>> front-end in addition to stop allocating a destination for memory fences<br>
>> on HSW-SKL in order to save a small amount of register pressure?<br>
>><br>
> ​It also fixes a fulsim error other than reducing register pressure.<br>
<br>
</div></div>On what platform?<br></blockquote><div><div class="gmail_default" style="font-family:verdana,sans-serif;font-size:small;display:inline">​I noticed this on Icelake. But, I'm sure it exists on CNL as well.​</div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<span class=""><br>
> I'll look in to how to stop allocating dst register for memory fences<br>
> and send out a v2. Thanks for the suggestion.  ​<br>
><br>
<br>
</span>In that case I think it would be nice to avoid duplicating the logic<br>
used to decide whether to enable the commit bit or not.  You can<br>
allocate the register conditionally in the visitor (providing a null<br>
register on HSW-SKL), and have brw_memory_fence() enable the commit bit<br>
if and only if dst is not null, that way we have the guarantee that the<br>
back-end and front-end are kept in sync.<br>
<div class="gmail_default" style="font-family:verdana,sans-serif;font-size:small;display:inline">​sounds good to me.​</div><br></blockquote><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
><br>
><br>
>><br>
>> ><br>
>> >> Cc'ing Curro.<br>
>> >><br>
>><br>
</blockquote></div><br></div></div>