Re: [Mesa-dev] [PATCH v2] nir: split SSBO min/max atomic instrinsics into signed/unsigned versions

2015-10-13 Thread Kenneth Graunke 
On Tuesday, September 29, 2015 09:25:27 AM Iago Toral Quiroga wrote:
> NIR is typeless so this is the only way to keep track of the
> type to select the proper atomic to use.
> 
> v2:
>   - Use imin,imax,umin,umax for the intrinsic names (Connor Abbott)
>   - Change message for unreachable paths (Michael Schellenberger)
> ---
>  src/glsl/nir/glsl_to_nir.cpp   | 22 ++
>  src/glsl/nir/nir_intrinsics.h  |  6 --
>  src/mesa/drivers/dri/i965/brw_fs_nir.cpp   | 20 ++--
>  src/mesa/drivers/dri/i965/brw_vec4_nir.cpp | 22 +++---
>  4 files changed, 43 insertions(+), 27 deletions(-)

Thanks, Iago!  This looks good to me.

Reviewed-by: Kenneth Graunke 


signature.asc
Description: This is a digitally signed message part.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v2] nir: split SSBO min/max atomic instrinsics into signed/unsigned versions

2015-10-13 Thread Kristian Høgsberg 
On Tue, Sep 29, 2015 at 09:25:27AM +0200, Iago Toral Quiroga wrote:
> NIR is typeless so this is the only way to keep track of the
> type to select the proper atomic to use.
> 
> v2:
>   - Use imin,imax,umin,umax for the intrinsic names (Connor Abbott)
>   - Change message for unreachable paths (Michael Schellenberger)

Looks good,

Reviewed-by: Kristian Høgsberg 

> ---
>  src/glsl/nir/glsl_to_nir.cpp   | 22 ++
>  src/glsl/nir/nir_intrinsics.h  |  6 --
>  src/mesa/drivers/dri/i965/brw_fs_nir.cpp   | 20 ++--
>  src/mesa/drivers/dri/i965/brw_vec4_nir.cpp | 22 +++---
>  4 files changed, 43 insertions(+), 27 deletions(-)
> 
> diff --git a/src/glsl/nir/glsl_to_nir.cpp b/src/glsl/nir/glsl_to_nir.cpp
> index f03a107..25f41a7 100644
> --- a/src/glsl/nir/glsl_to_nir.cpp
> +++ b/src/glsl/nir/glsl_to_nir.cpp
> @@ -662,9 +662,21 @@ nir_visitor::visit(ir_call *ir)
>} else if (strcmp(ir->callee_name(), 
> "__intrinsic_ssbo_atomic_xor_internal") == 0) {
>   op = nir_intrinsic_ssbo_atomic_xor;
>} else if (strcmp(ir->callee_name(), 
> "__intrinsic_ssbo_atomic_min_internal") == 0) {
> - op = nir_intrinsic_ssbo_atomic_min;
> + assert(ir->return_deref);
> + if (ir->return_deref->type == glsl_type::int_type)
> +op = nir_intrinsic_ssbo_atomic_imin;
> + else if (ir->return_deref->type == glsl_type::uint_type)
> +op = nir_intrinsic_ssbo_atomic_umin;
> + else
> +unreachable("Invalid type");
>} else if (strcmp(ir->callee_name(), 
> "__intrinsic_ssbo_atomic_max_internal") == 0) {
> - op = nir_intrinsic_ssbo_atomic_max;
> + assert(ir->return_deref);
> + if (ir->return_deref->type == glsl_type::int_type)
> +op = nir_intrinsic_ssbo_atomic_imax;
> + else if (ir->return_deref->type == glsl_type::uint_type)
> +op = nir_intrinsic_ssbo_atomic_umax;
> + else
> +unreachable("Invalid type");
>} else if (strcmp(ir->callee_name(), 
> "__intrinsic_ssbo_atomic_exchange_internal") == 0) {
>   op = nir_intrinsic_ssbo_atomic_exchange;
>} else if (strcmp(ir->callee_name(), 
> "__intrinsic_ssbo_atomic_comp_swap_internal") == 0) {
> @@ -874,8 +886,10 @@ nir_visitor::visit(ir_call *ir)
>   break;
>}
>case nir_intrinsic_ssbo_atomic_add:
> -  case nir_intrinsic_ssbo_atomic_min:
> -  case nir_intrinsic_ssbo_atomic_max:
> +  case nir_intrinsic_ssbo_atomic_imin:
> +  case nir_intrinsic_ssbo_atomic_umin:
> +  case nir_intrinsic_ssbo_atomic_imax:
> +  case nir_intrinsic_ssbo_atomic_umax:
>case nir_intrinsic_ssbo_atomic_and:
>case nir_intrinsic_ssbo_atomic_or:
>case nir_intrinsic_ssbo_atomic_xor:
> diff --git a/src/glsl/nir/nir_intrinsics.h b/src/glsl/nir/nir_intrinsics.h
> index 06f1b02..ff42bb2 100644
> --- a/src/glsl/nir/nir_intrinsics.h
> +++ b/src/glsl/nir/nir_intrinsics.h
> @@ -174,8 +174,10 @@ INTRINSIC(image_samples, 0, ARR(), true, 1, 1, 0,
>   * 3: For CompSwap only: the second data parameter.
>   */
>  INTRINSIC(ssbo_atomic_add, 3, ARR(1, 1, 1), true, 1, 0, 0, 0)
> -INTRINSIC(ssbo_atomic_min, 3, ARR(1, 1, 1), true, 1, 0, 0, 0)
> -INTRINSIC(ssbo_atomic_max, 3, ARR(1, 1, 1), true, 1, 0, 0, 0)
> +INTRINSIC(ssbo_atomic_imin, 3, ARR(1, 1, 1), true, 1, 0, 0, 0)
> +INTRINSIC(ssbo_atomic_umin, 3, ARR(1, 1, 1), true, 1, 0, 0, 0)
> +INTRINSIC(ssbo_atomic_imax, 3, ARR(1, 1, 1), true, 1, 0, 0, 0)
> +INTRINSIC(ssbo_atomic_umax, 3, ARR(1, 1, 1), true, 1, 0, 0, 0)
>  INTRINSIC(ssbo_atomic_and, 3, ARR(1, 1, 1), true, 1, 0, 0, 0)
>  INTRINSIC(ssbo_atomic_or, 3, ARR(1, 1, 1), true, 1, 0, 0, 0)
>  INTRINSIC(ssbo_atomic_xor, 3, ARR(1, 1, 1), true, 1, 0, 0, 0)
> diff --git a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp 
> b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> index a2bc5c6..0b9555c 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> @@ -1870,17 +1870,17 @@ fs_visitor::nir_emit_intrinsic(const fs_builder &bld, 
> nir_intrinsic_instr *instr
> case nir_intrinsic_ssbo_atomic_add:
>nir_emit_ssbo_atomic(bld, BRW_AOP_ADD, instr);
>break;
> -   case nir_intrinsic_ssbo_atomic_min:
> -  if (dest.type == BRW_REGISTER_TYPE_D)
> - nir_emit_ssbo_atomic(bld, BRW_AOP_IMIN, instr);
> -  else
> - nir_emit_ssbo_atomic(bld, BRW_AOP_UMIN, instr);
> +   case nir_intrinsic_ssbo_atomic_imin:
> +  nir_emit_ssbo_atomic(bld, BRW_AOP_IMIN, instr);
>break;
> -   case nir_intrinsic_ssbo_atomic_max:
> -  if (dest.type == BRW_REGISTER_TYPE_D)
> - nir_emit_ssbo_atomic(bld, BRW_AOP_IMAX, instr);
> -  else
> - nir_emit_ssbo_atomic(bld, BRW_AOP_UMAX, instr);
> +   case nir_intrinsic_ssbo_atomic_umin:
> +  nir_emit_ssbo_atomic(bld, BRW_AOP_UMIN, instr);
> +  break;
> +   case nir_intrinsic_ssbo_atomic_imax:
> +

Re: [Mesa-dev] [PATCH v2] nir: split SSBO min/max atomic instrinsics into signed/unsigned versions

2015-10-05 Thread Iago Toral 
Hi,

On Tue, 2015-09-29 at 09:25 +0200, Iago Toral Quiroga wrote:
> NIR is typeless so this is the only way to keep track of the
> type to select the proper atomic to use.
> 
> v2:
>   - Use imin,imax,umin,umax for the intrinsic names (Connor Abbott)
>   - Change message for unreachable paths (Michael Schellenberger)

Since this changes the intrinsics we define for these things I'd like to
push this sooner rather than later. There have been no comments to v2 so
far, any of you feel like giving the Rb to this?

Thanks
Iago

> ---
>  src/glsl/nir/glsl_to_nir.cpp   | 22 ++
>  src/glsl/nir/nir_intrinsics.h  |  6 --
>  src/mesa/drivers/dri/i965/brw_fs_nir.cpp   | 20 ++--
>  src/mesa/drivers/dri/i965/brw_vec4_nir.cpp | 22 +++---
>  4 files changed, 43 insertions(+), 27 deletions(-)
> 
> diff --git a/src/glsl/nir/glsl_to_nir.cpp b/src/glsl/nir/glsl_to_nir.cpp
> index f03a107..25f41a7 100644
> --- a/src/glsl/nir/glsl_to_nir.cpp
> +++ b/src/glsl/nir/glsl_to_nir.cpp
> @@ -662,9 +662,21 @@ nir_visitor::visit(ir_call *ir)
>} else if (strcmp(ir->callee_name(), 
> "__intrinsic_ssbo_atomic_xor_internal") == 0) {
>   op = nir_intrinsic_ssbo_atomic_xor;
>} else if (strcmp(ir->callee_name(), 
> "__intrinsic_ssbo_atomic_min_internal") == 0) {
> - op = nir_intrinsic_ssbo_atomic_min;
> + assert(ir->return_deref);
> + if (ir->return_deref->type == glsl_type::int_type)
> +op = nir_intrinsic_ssbo_atomic_imin;
> + else if (ir->return_deref->type == glsl_type::uint_type)
> +op = nir_intrinsic_ssbo_atomic_umin;
> + else
> +unreachable("Invalid type");
>} else if (strcmp(ir->callee_name(), 
> "__intrinsic_ssbo_atomic_max_internal") == 0) {
> - op = nir_intrinsic_ssbo_atomic_max;
> + assert(ir->return_deref);
> + if (ir->return_deref->type == glsl_type::int_type)
> +op = nir_intrinsic_ssbo_atomic_imax;
> + else if (ir->return_deref->type == glsl_type::uint_type)
> +op = nir_intrinsic_ssbo_atomic_umax;
> + else
> +unreachable("Invalid type");
>} else if (strcmp(ir->callee_name(), 
> "__intrinsic_ssbo_atomic_exchange_internal") == 0) {
>   op = nir_intrinsic_ssbo_atomic_exchange;
>} else if (strcmp(ir->callee_name(), 
> "__intrinsic_ssbo_atomic_comp_swap_internal") == 0) {
> @@ -874,8 +886,10 @@ nir_visitor::visit(ir_call *ir)
>   break;
>}
>case nir_intrinsic_ssbo_atomic_add:
> -  case nir_intrinsic_ssbo_atomic_min:
> -  case nir_intrinsic_ssbo_atomic_max:
> +  case nir_intrinsic_ssbo_atomic_imin:
> +  case nir_intrinsic_ssbo_atomic_umin:
> +  case nir_intrinsic_ssbo_atomic_imax:
> +  case nir_intrinsic_ssbo_atomic_umax:
>case nir_intrinsic_ssbo_atomic_and:
>case nir_intrinsic_ssbo_atomic_or:
>case nir_intrinsic_ssbo_atomic_xor:
> diff --git a/src/glsl/nir/nir_intrinsics.h b/src/glsl/nir/nir_intrinsics.h
> index 06f1b02..ff42bb2 100644
> --- a/src/glsl/nir/nir_intrinsics.h
> +++ b/src/glsl/nir/nir_intrinsics.h
> @@ -174,8 +174,10 @@ INTRINSIC(image_samples, 0, ARR(), true, 1, 1, 0,
>   * 3: For CompSwap only: the second data parameter.
>   */
>  INTRINSIC(ssbo_atomic_add, 3, ARR(1, 1, 1), true, 1, 0, 0, 0)
> -INTRINSIC(ssbo_atomic_min, 3, ARR(1, 1, 1), true, 1, 0, 0, 0)
> -INTRINSIC(ssbo_atomic_max, 3, ARR(1, 1, 1), true, 1, 0, 0, 0)
> +INTRINSIC(ssbo_atomic_imin, 3, ARR(1, 1, 1), true, 1, 0, 0, 0)
> +INTRINSIC(ssbo_atomic_umin, 3, ARR(1, 1, 1), true, 1, 0, 0, 0)
> +INTRINSIC(ssbo_atomic_imax, 3, ARR(1, 1, 1), true, 1, 0, 0, 0)
> +INTRINSIC(ssbo_atomic_umax, 3, ARR(1, 1, 1), true, 1, 0, 0, 0)
>  INTRINSIC(ssbo_atomic_and, 3, ARR(1, 1, 1), true, 1, 0, 0, 0)
>  INTRINSIC(ssbo_atomic_or, 3, ARR(1, 1, 1), true, 1, 0, 0, 0)
>  INTRINSIC(ssbo_atomic_xor, 3, ARR(1, 1, 1), true, 1, 0, 0, 0)
> diff --git a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp 
> b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> index a2bc5c6..0b9555c 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> @@ -1870,17 +1870,17 @@ fs_visitor::nir_emit_intrinsic(const fs_builder &bld, 
> nir_intrinsic_instr *instr
> case nir_intrinsic_ssbo_atomic_add:
>nir_emit_ssbo_atomic(bld, BRW_AOP_ADD, instr);
>break;
> -   case nir_intrinsic_ssbo_atomic_min:
> -  if (dest.type == BRW_REGISTER_TYPE_D)
> - nir_emit_ssbo_atomic(bld, BRW_AOP_IMIN, instr);
> -  else
> - nir_emit_ssbo_atomic(bld, BRW_AOP_UMIN, instr);
> +   case nir_intrinsic_ssbo_atomic_imin:
> +  nir_emit_ssbo_atomic(bld, BRW_AOP_IMIN, instr);
>break;
> -   case nir_intrinsic_ssbo_atomic_max:
> -  if (dest.type == BRW_REGISTER_TYPE_D)
> - nir_emit_ssbo_atomic(bld, BRW_AOP_IMAX, instr);
> -  else
> - nir_emit_ssbo_atomic(bld, BRW_AOP_UMAX, instr);
>

Re: [Mesa-dev] [PATCH v2] nir: split SSBO min/max atomic instrinsics into signed/unsigned versions

2015-10-04 Thread Iago Toral 
Hi Markus,

I noticed that you did not reply to mesa-dev in your original e-mail so
I am CCing the list now so we keep the discussion here.

On Mon, 2015-10-05 at 08:07 +0200, Iago Toral wrote:
> Hi Markus,
> 
> On Sun, 2015-10-04 at 18:15 +0200, Markus Wick wrote:
> > Hi Iago,
> > 
> > I've tried your SSBO patch with splitted signed / unsigned handling with 
> > dolphin-emu and it did work fine, so feel free to add
> > Tested-by: Markus Wick 
> 
> Thanks!
> 
> > But I have another issue with SSBO on master. I get "First argument to 
> > atomic function must be a buffer variable" because of this code: "buffer 
> > {ivec4 a;} ; ... atomicMax(a[0], 0); ...".
> 
> 
> 
> > Ilia has told me on #dri-devel about having the same issue with 
> > tesselation:
> > imirkin_> the second issue sounds fun though... we had issues like that 
> > with tess for a while
> > imirkin_> probably the same helpers can be reused to peer through the 
> > swizzles
> > 
> > Do you know a way how to fix it?
> 
> I'll look into it and reply here with my findings (or a patch). Thanks
> for reporting it!
> 
> Iago
> 
> > Thanks
> > 
> > Markus - degasus
> > 
> 


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

[Mesa-dev] [PATCH v2] nir: split SSBO min/max atomic instrinsics into signed/unsigned versions

2015-09-29 Thread Iago Toral Quiroga 
NIR is typeless so this is the only way to keep track of the
type to select the proper atomic to use.

v2:
  - Use imin,imax,umin,umax for the intrinsic names (Connor Abbott)
  - Change message for unreachable paths (Michael Schellenberger)
---
 src/glsl/nir/glsl_to_nir.cpp   | 22 ++
 src/glsl/nir/nir_intrinsics.h  |  6 --
 src/mesa/drivers/dri/i965/brw_fs_nir.cpp   | 20 ++--
 src/mesa/drivers/dri/i965/brw_vec4_nir.cpp | 22 +++---
 4 files changed, 43 insertions(+), 27 deletions(-)

diff --git a/src/glsl/nir/glsl_to_nir.cpp b/src/glsl/nir/glsl_to_nir.cpp
index f03a107..25f41a7 100644
--- a/src/glsl/nir/glsl_to_nir.cpp
+++ b/src/glsl/nir/glsl_to_nir.cpp
@@ -662,9 +662,21 @@ nir_visitor::visit(ir_call *ir)
   } else if (strcmp(ir->callee_name(), 
"__intrinsic_ssbo_atomic_xor_internal") == 0) {
  op = nir_intrinsic_ssbo_atomic_xor;
   } else if (strcmp(ir->callee_name(), 
"__intrinsic_ssbo_atomic_min_internal") == 0) {
- op = nir_intrinsic_ssbo_atomic_min;
+ assert(ir->return_deref);
+ if (ir->return_deref->type == glsl_type::int_type)
+op = nir_intrinsic_ssbo_atomic_imin;
+ else if (ir->return_deref->type == glsl_type::uint_type)
+op = nir_intrinsic_ssbo_atomic_umin;
+ else
+unreachable("Invalid type");
   } else if (strcmp(ir->callee_name(), 
"__intrinsic_ssbo_atomic_max_internal") == 0) {
- op = nir_intrinsic_ssbo_atomic_max;
+ assert(ir->return_deref);
+ if (ir->return_deref->type == glsl_type::int_type)
+op = nir_intrinsic_ssbo_atomic_imax;
+ else if (ir->return_deref->type == glsl_type::uint_type)
+op = nir_intrinsic_ssbo_atomic_umax;
+ else
+unreachable("Invalid type");
   } else if (strcmp(ir->callee_name(), 
"__intrinsic_ssbo_atomic_exchange_internal") == 0) {
  op = nir_intrinsic_ssbo_atomic_exchange;
   } else if (strcmp(ir->callee_name(), 
"__intrinsic_ssbo_atomic_comp_swap_internal") == 0) {
@@ -874,8 +886,10 @@ nir_visitor::visit(ir_call *ir)
  break;
   }
   case nir_intrinsic_ssbo_atomic_add:
-  case nir_intrinsic_ssbo_atomic_min:
-  case nir_intrinsic_ssbo_atomic_max:
+  case nir_intrinsic_ssbo_atomic_imin:
+  case nir_intrinsic_ssbo_atomic_umin:
+  case nir_intrinsic_ssbo_atomic_imax:
+  case nir_intrinsic_ssbo_atomic_umax:
   case nir_intrinsic_ssbo_atomic_and:
   case nir_intrinsic_ssbo_atomic_or:
   case nir_intrinsic_ssbo_atomic_xor:
diff --git a/src/glsl/nir/nir_intrinsics.h b/src/glsl/nir/nir_intrinsics.h
index 06f1b02..ff42bb2 100644
--- a/src/glsl/nir/nir_intrinsics.h
+++ b/src/glsl/nir/nir_intrinsics.h
@@ -174,8 +174,10 @@ INTRINSIC(image_samples, 0, ARR(), true, 1, 1, 0,
  * 3: For CompSwap only: the second data parameter.
  */
 INTRINSIC(ssbo_atomic_add, 3, ARR(1, 1, 1), true, 1, 0, 0, 0)
-INTRINSIC(ssbo_atomic_min, 3, ARR(1, 1, 1), true, 1, 0, 0, 0)
-INTRINSIC(ssbo_atomic_max, 3, ARR(1, 1, 1), true, 1, 0, 0, 0)
+INTRINSIC(ssbo_atomic_imin, 3, ARR(1, 1, 1), true, 1, 0, 0, 0)
+INTRINSIC(ssbo_atomic_umin, 3, ARR(1, 1, 1), true, 1, 0, 0, 0)
+INTRINSIC(ssbo_atomic_imax, 3, ARR(1, 1, 1), true, 1, 0, 0, 0)
+INTRINSIC(ssbo_atomic_umax, 3, ARR(1, 1, 1), true, 1, 0, 0, 0)
 INTRINSIC(ssbo_atomic_and, 3, ARR(1, 1, 1), true, 1, 0, 0, 0)
 INTRINSIC(ssbo_atomic_or, 3, ARR(1, 1, 1), true, 1, 0, 0, 0)
 INTRINSIC(ssbo_atomic_xor, 3, ARR(1, 1, 1), true, 1, 0, 0, 0)
diff --git a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp 
b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
index a2bc5c6..0b9555c 100644
--- a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
+++ b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
@@ -1870,17 +1870,17 @@ fs_visitor::nir_emit_intrinsic(const fs_builder &bld, 
nir_intrinsic_instr *instr
case nir_intrinsic_ssbo_atomic_add:
   nir_emit_ssbo_atomic(bld, BRW_AOP_ADD, instr);
   break;
-   case nir_intrinsic_ssbo_atomic_min:
-  if (dest.type == BRW_REGISTER_TYPE_D)
- nir_emit_ssbo_atomic(bld, BRW_AOP_IMIN, instr);
-  else
- nir_emit_ssbo_atomic(bld, BRW_AOP_UMIN, instr);
+   case nir_intrinsic_ssbo_atomic_imin:
+  nir_emit_ssbo_atomic(bld, BRW_AOP_IMIN, instr);
   break;
-   case nir_intrinsic_ssbo_atomic_max:
-  if (dest.type == BRW_REGISTER_TYPE_D)
- nir_emit_ssbo_atomic(bld, BRW_AOP_IMAX, instr);
-  else
- nir_emit_ssbo_atomic(bld, BRW_AOP_UMAX, instr);
+   case nir_intrinsic_ssbo_atomic_umin:
+  nir_emit_ssbo_atomic(bld, BRW_AOP_UMIN, instr);
+  break;
+   case nir_intrinsic_ssbo_atomic_imax:
+  nir_emit_ssbo_atomic(bld, BRW_AOP_IMAX, instr);
+  break;
+   case nir_intrinsic_ssbo_atomic_umax:
+  nir_emit_ssbo_atomic(bld, BRW_AOP_UMAX, instr);
   break;
case nir_intrinsic_ssbo_atomic_and:
   nir_emit_ssbo_atomic(bld, BRW_AOP_AND, instr);
diff --git a/src/mesa/drivers/dri/i965/brw_vec4_nir