Re: [Mesa-dev] [PATCHv4 4/7] nir: add shader_clock intrinsic

[Emil Velikov]

On 20 October 2015 at 19:58, Connor Abbott  wrote:
> On Tue, Oct 20, 2015 at 12:55 PM, Emil Velikov  
> wrote:
[snip]
>> +/*
>> + * Shader clock intrinsic with semantics analogous to the clock2x32ARB()
>> + * GLSL intrinsic.
>> + * The latter can be used as code motion barrier, which is currently not
>> + * feasible with NIR, thus we can eliminate the intrinsic when the return
>> + * value is unused.
>
> Just a small bikeshedding thing: technically, even if we were to make
> shader_clock a code motion barrier like the spec asks for, we could
> still delete it if its result is unused because then nobody will
> notice if we move code around it. Get rid of the "thus we can
> eliminate..." bit and this is
>
Indeed you're correct. It won't make much sense to keep in around.

Thanks for the suggestions and review.
Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCHv4 4/7] nir: add shader_clock intrinsic

[Connor Abbott]

On Tue, Oct 20, 2015 at 12:55 PM, Emil Velikov  wrote:
> v2: Add flags and inline comment/description.
> v3: None of the input/outputs are variables
> v4: Drop clockARB reference, relate code motion barrier comment wrt
> intrinsic flag.
>
> Signed-off-by: Emil Velikov 
> ---
>
> Matt,
>
> Hopefully the updated comment makes more sense (the clockARB
> reference was an unintentional left over). If not I'll just nuke it as
> you suggested.
>
> -Emil
>
>
>  src/glsl/nir/glsl_to_nir.cpp  | 6 ++
>  src/glsl/nir/nir_intrinsics.h | 9 +
>  2 files changed, 15 insertions(+)
>
> diff --git a/src/glsl/nir/glsl_to_nir.cpp b/src/glsl/nir/glsl_to_nir.cpp
> index cf5bb93..9dd3d07 100644
> --- a/src/glsl/nir/glsl_to_nir.cpp
> +++ b/src/glsl/nir/glsl_to_nir.cpp
> @@ -685,6 +685,8 @@ nir_visitor::visit(ir_call *ir)
>   op = nir_intrinsic_ssbo_atomic_exchange;
>} else if (strcmp(ir->callee_name(), 
> "__intrinsic_ssbo_atomic_comp_swap_internal") == 0) {
>   op = nir_intrinsic_ssbo_atomic_comp_swap;
> +  } else if (strcmp(ir->callee_name(), "__intrinsic_shader_clock") == 0) 
> {
> + op = nir_intrinsic_shader_clock;
>} else {
>   unreachable("not reached");
>}
> @@ -789,6 +791,10 @@ nir_visitor::visit(ir_call *ir)
>case nir_intrinsic_memory_barrier:
>   nir_instr_insert_after_cf_list(this->cf_node_list, >instr);
>   break;
> +  case nir_intrinsic_shader_clock:
> + nir_ssa_dest_init(>instr, >dest, 1, NULL);
> + nir_instr_insert_after_cf_list(this->cf_node_list, >instr);
> + break;
>case nir_intrinsic_store_ssbo: {
>   exec_node *param = ir->actual_parameters.get_head();
>   ir_rvalue *block = ((ir_instruction *)param)->as_rvalue();
> diff --git a/src/glsl/nir/nir_intrinsics.h b/src/glsl/nir/nir_intrinsics.h
> index 49bf3b2..7485070 100644
> --- a/src/glsl/nir/nir_intrinsics.h
> +++ b/src/glsl/nir/nir_intrinsics.h
> @@ -83,6 +83,15 @@ BARRIER(discard)
>   */
>  BARRIER(memory_barrier)
>
> +/*
> + * Shader clock intrinsic with semantics analogous to the clock2x32ARB()
> + * GLSL intrinsic.
> + * The latter can be used as code motion barrier, which is currently not
> + * feasible with NIR, thus we can eliminate the intrinsic when the return
> + * value is unused.

Just a small bikeshedding thing: technically, even if we were to make
shader_clock a code motion barrier like the spec asks for, we could
still delete it if its result is unused because then nobody will
notice if we move code around it. Get rid of the "thus we can
eliminate..." bit and this is

Reviewed-by: Connor Abbott 

> + */
> +INTRINSIC(shader_clock, 0, ARR(), true, 1, 0, 0, NIR_INTRINSIC_CAN_ELIMINATE)
> +
>  /** A conditional discard, with a single boolean source. */
>  INTRINSIC(discard_if, 1, ARR(1), false, 0, 0, 0, 0)
>
> --
> 2.6.1
>
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCHv4 4/7] nir: add shader_clock intrinsic

[Emil Velikov]

v2: Add flags and inline comment/description.
v3: None of the input/outputs are variables
v4: Drop clockARB reference, relate code motion barrier comment wrt
intrinsic flag.

Signed-off-by: Emil Velikov 
---

Matt,

Hopefully the updated comment makes more sense (the clockARB 
reference was an unintentional left over). If not I'll just nuke it as 
you suggested.

-Emil


 src/glsl/nir/glsl_to_nir.cpp  | 6 ++
 src/glsl/nir/nir_intrinsics.h | 9 +
 2 files changed, 15 insertions(+)

diff --git a/src/glsl/nir/glsl_to_nir.cpp b/src/glsl/nir/glsl_to_nir.cpp
index cf5bb93..9dd3d07 100644
--- a/src/glsl/nir/glsl_to_nir.cpp
+++ b/src/glsl/nir/glsl_to_nir.cpp
@@ -685,6 +685,8 @@ nir_visitor::visit(ir_call *ir)
  op = nir_intrinsic_ssbo_atomic_exchange;
   } else if (strcmp(ir->callee_name(), 
"__intrinsic_ssbo_atomic_comp_swap_internal") == 0) {
  op = nir_intrinsic_ssbo_atomic_comp_swap;
+  } else if (strcmp(ir->callee_name(), "__intrinsic_shader_clock") == 0) {
+ op = nir_intrinsic_shader_clock;
   } else {
  unreachable("not reached");
   }
@@ -789,6 +791,10 @@ nir_visitor::visit(ir_call *ir)
   case nir_intrinsic_memory_barrier:
  nir_instr_insert_after_cf_list(this->cf_node_list, >instr);
  break;
+  case nir_intrinsic_shader_clock:
+ nir_ssa_dest_init(>instr, >dest, 1, NULL);
+ nir_instr_insert_after_cf_list(this->cf_node_list, >instr);
+ break;
   case nir_intrinsic_store_ssbo: {
  exec_node *param = ir->actual_parameters.get_head();
  ir_rvalue *block = ((ir_instruction *)param)->as_rvalue();
diff --git a/src/glsl/nir/nir_intrinsics.h b/src/glsl/nir/nir_intrinsics.h
index 49bf3b2..7485070 100644
--- a/src/glsl/nir/nir_intrinsics.h
+++ b/src/glsl/nir/nir_intrinsics.h
@@ -83,6 +83,15 @@ BARRIER(discard)
  */
 BARRIER(memory_barrier)
 
+/*
+ * Shader clock intrinsic with semantics analogous to the clock2x32ARB()
+ * GLSL intrinsic.
+ * The latter can be used as code motion barrier, which is currently not
+ * feasible with NIR, thus we can eliminate the intrinsic when the return
+ * value is unused.
+ */
+INTRINSIC(shader_clock, 0, ARR(), true, 1, 0, 0, NIR_INTRINSIC_CAN_ELIMINATE)
+
 /** A conditional discard, with a single boolean source. */
 INTRINSIC(discard_if, 1, ARR(1), false, 0, 0, 0, 0)
 
-- 
2.6.1

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev