<div dir="ltr">Reviewed-by: Jason Ekstrand <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>><br></div><br><div class="gmail_quote"><div dir="ltr">On Mon, Jul 9, 2018 at 2:09 AM Iago Toral <<a href="mailto:itoral@igalia.com">itoral@igalia.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Any feedback about this? We need this to fix some new CTS tests.<br>
<br>
On Thu, 2018-06-21 at 13:52 +0200, Iago Toral Quiroga wrote:<br>
> Until now we have assumed that we could skip emitting these barriers<br>
> in the general case based on empirical testing and a few assumptions<br>
> detailed in a comment in the driver code, however, recent CTS tests<br>
> have showed that we actually need them to produce correct behavior.<br>
> ---<br>
>  src/intel/compiler/brw_fs_nir.cpp | 25 ++-----------------------<br>
>  1 file changed, 2 insertions(+), 23 deletions(-)<br>
> <br>
> diff --git a/src/intel/compiler/brw_fs_nir.cpp<br>
> b/src/intel/compiler/brw_fs_nir.cpp<br>
> index 0abb4798e70..d0648c89865 100644<br>
> --- a/src/intel/compiler/brw_fs_nir.cpp<br>
> +++ b/src/intel/compiler/brw_fs_nir.cpp<br>
> @@ -3884,6 +3884,8 @@ fs_visitor::nir_emit_intrinsic(const fs_builder<br>
> &bld, nir_intrinsic_instr *instr<br>
>        break;<br>
>     }<br>
>  <br>
> +   case nir_intrinsic_group_memory_barrier:<br>
> +   case nir_intrinsic_memory_barrier_shared:<br>
>     case nir_intrinsic_memory_barrier_atomic_counter:<br>
>     case nir_intrinsic_memory_barrier_buffer:<br>
>     case nir_intrinsic_memory_barrier_image:<br>
> @@ -3895,29 +3897,6 @@ fs_visitor::nir_emit_intrinsic(const<br>
> fs_builder &bld, nir_intrinsic_instr *instr<br>
>        break;<br>
>     }<br>
>  <br>
> -   case nir_intrinsic_group_memory_barrier:<br>
> -   case nir_intrinsic_memory_barrier_shared:<br>
> -      /* We treat these workgroup-level barriers as no-ops.  This<br>
> should be<br>
> -       * safe at present and as long as:<br>
> -       *<br>
> -       *  - Memory access instructions are not subsequently<br>
> reordered by the<br>
> -       *    compiler back-end.<br>
> -       *<br>
> -       *  - All threads from a given compute shader workgroup fit<br>
> within a<br>
> -       *    single subslice and therefore talk to the same HDC<br>
> shared unit<br>
> -       *    what supposedly guarantees ordering and coherency<br>
> between threads<br>
> -       *    from the same workgroup.  This may change in the future<br>
> when we<br>
> -       *    start splitting workgroups across multiple subslices.<br>
> -       *<br>
> -       *  - The context is not in fault-and-stream mode, which could<br>
> cause<br>
> -       *    memory transactions (including to SLM) prior to the<br>
> barrier to be<br>
> -       *    replayed after the barrier if a pagefault occurs.  This<br>
> shouldn't<br>
> -       *    be a problem up to and including SKL because fault-and-<br>
> stream is<br>
> -       *    not usable due to hardware issues, but that's likely to<br>
> change in<br>
> -       *    the future.<br>
> -       */<br>
> -      break;<br>
> -<br>
>     case nir_intrinsic_shader_clock: {<br>
>        /* We cannot do anything if there is an event, so ignore it<br>
> for now */<br>
>        const fs_reg shader_clock = get_timestamp(bld);<br>
</blockquote></div>