Re: [Mesa-dev] [PATCH 2/2] i965: Fix shared atomic intrinsics to pay attention to base.

[Jason Ekstrand]

On Jul 18, 2016 3:49 PM, "Kenneth Graunke"  wrote:
>
> Signed-off-by: Kenneth Graunke 
> ---
>  src/mesa/drivers/dri/i965/brw_fs_nir.cpp | 15 +--
>  1 file changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> index 6265dc6..a39c37e 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> @@ -4177,13 +4177,24 @@ fs_visitor::nir_emit_shared_atomic(const
fs_builder ,
>dest = get_nir_dest(instr->dest);
>
> fs_reg surface = brw_imm_ud(GEN7_BTI_SLM);
> -   fs_reg offset = get_nir_src(instr->src[0]);
> +   fs_reg offset;
> fs_reg data1 = get_nir_src(instr->src[1]);
> fs_reg data2;
> if (op == BRW_AOP_CMPWR)
>data2 = get_nir_src(instr->src[2]);
>
> -   /* Emit the actual atomic operation operation */
> +   /* Get the offset */
> +   nir_const_value *const_offset = nir_src_as_const_value(instr->src[0]);
> +   if (const_offset) {
> +  offset = brw_imm_ud(instr->const_index[0] + const_offset->u32[0]);

Should we be using nir_intrinsic_base here instead of accessing the
const_index directly?

> +   } else {
> +  offset = vgrf(glsl_type::uint_type);
> +  bld.ADD(offset,
> + retype(get_nir_src(instr->src[0]), BRW_REGISTER_TYPE_UD),
> + brw_imm_ud(instr->const_index[0]));

Same here

> +   }
> +
> +   /* Emit the actua atomic operation operation */
>
> fs_reg atomic_result = emit_untyped_atomic(bld, surface, offset,
>data1, data2,
> --
> 2.9.0
>
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 2/2] i965: Fix shared atomic intrinsics to pay attention to base.

[Kenneth Graunke]

On Tuesday, July 19, 2016 10:23:03 AM PDT Timothy Arceri wrote:
> On Mon, 2016-07-18 at 15:49 -0700, Kenneth Graunke wrote:
> So this fixes a bug with indirects right? Is there a piglit test for
> this?

Not exactly.  Right now, GLSL lowers shared variable access at the
GLSL IR level, and when we translate the GLSL IR intrinsics to NIR,
we always set base to 0 and put everything in offset.  So Piglit
wouldn't have hit this.

Vulkan lowers shared variables in NIR, and was actually using a
non-zero base.  Vulkan tests could have hit this, but I don't think
any actually did.

I recall Jordan saying he ran into some issues when trying to make
GLSL use the NIR-based lowering, so maybe Piglit actually did hit
this base problem.  Not sure.

> With the typo Ilia pointed out fixed, both are:
> 
> Reviewed-by: Timothy Arceri 

Thanks!


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

Re: [Mesa-dev] [PATCH 2/2] i965: Fix shared atomic intrinsics to pay attention to base.

[Timothy Arceri]

On Mon, 2016-07-18 at 15:49 -0700, Kenneth Graunke wrote:

So this fixes a bug with indirects right? Is there a piglit test for
this?

With the typo Ilia pointed out fixed, both are:

Reviewed-by: Timothy Arceri 


> Signed-off-by: Kenneth Graunke 
> ---
>  src/mesa/drivers/dri/i965/brw_fs_nir.cpp | 15 +--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> index 6265dc6..a39c37e 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> @@ -4177,13 +4177,24 @@ fs_visitor::nir_emit_shared_atomic(const
> fs_builder ,
>    dest = get_nir_dest(instr->dest);
>  
> fs_reg surface = brw_imm_ud(GEN7_BTI_SLM);
> -   fs_reg offset = get_nir_src(instr->src[0]);
> +   fs_reg offset;
> fs_reg data1 = get_nir_src(instr->src[1]);
> fs_reg data2;
> if (op == BRW_AOP_CMPWR)
>    data2 = get_nir_src(instr->src[2]);
>  
> -   /* Emit the actual atomic operation operation */
> +   /* Get the offset */
> +   nir_const_value *const_offset = nir_src_as_const_value(instr-
> >src[0]);
> +   if (const_offset) {
> +  offset = brw_imm_ud(instr->const_index[0] + const_offset-
> >u32[0]);
> +   } else {
> +  offset = vgrf(glsl_type::uint_type);
> +  bld.ADD(offset,
> +   retype(get_nir_src(instr->src[0]),
> BRW_REGISTER_TYPE_UD),
> +   brw_imm_ud(instr->const_index[0]));
> +   }
> +
> +   /* Emit the actua atomic operation operation */
>  
> fs_reg atomic_result = emit_untyped_atomic(bld, surface, offset,
>    data1, data2,
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 2/2] i965: Fix shared atomic intrinsics to pay attention to base.

[Timothy Arceri]

On Mon, 2016-07-18 at 15:49 -0700, Kenneth Graunke wrote:

So this fixes a bug with indirects right? Is there a piglit test for
this?

With the typo Ilia pointed out fixed.

Reviewed-by: Timothy Arceri 


> Signed-off-by: Kenneth Graunke 
> ---
>  src/mesa/drivers/dri/i965/brw_fs_nir.cpp | 15 +--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> index 6265dc6..a39c37e 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> @@ -4177,13 +4177,24 @@ fs_visitor::nir_emit_shared_atomic(const
> fs_builder ,
>    dest = get_nir_dest(instr->dest);
>  
> fs_reg surface = brw_imm_ud(GEN7_BTI_SLM);
> -   fs_reg offset = get_nir_src(instr->src[0]);
> +   fs_reg offset;
> fs_reg data1 = get_nir_src(instr->src[1]);
> fs_reg data2;
> if (op == BRW_AOP_CMPWR)
>    data2 = get_nir_src(instr->src[2]);
>  
> -   /* Emit the actual atomic operation operation */
> +   /* Get the offset */
> +   nir_const_value *const_offset = nir_src_as_const_value(instr-
> >src[0]);
> +   if (const_offset) {
> +  offset = brw_imm_ud(instr->const_index[0] + const_offset-
> >u32[0]);
> +   } else {
> +  offset = vgrf(glsl_type::uint_type);
> +  bld.ADD(offset,
> +   retype(get_nir_src(instr->src[0]),
> BRW_REGISTER_TYPE_UD),
> +   brw_imm_ud(instr->const_index[0]));
> +   }
> +
> +   /* Emit the actua atomic operation operation */
>  
> fs_reg atomic_result = emit_untyped_atomic(bld, surface, offset,
>    data1, data2,
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 2/2] i965: Fix shared atomic intrinsics to pay attention to base.

[Ilia Mirkin]

On Mon, Jul 18, 2016 at 6:49 PM, Kenneth Graunke  wrote:
> Signed-off-by: Kenneth Graunke 
> ---
>  src/mesa/drivers/dri/i965/brw_fs_nir.cpp | 15 +--
>  1 file changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp 
> b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> index 6265dc6..a39c37e 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> @@ -4177,13 +4177,24 @@ fs_visitor::nir_emit_shared_atomic(const fs_builder 
> ,
>dest = get_nir_dest(instr->dest);
>
> fs_reg surface = brw_imm_ud(GEN7_BTI_SLM);
> -   fs_reg offset = get_nir_src(instr->src[0]);
> +   fs_reg offset;
> fs_reg data1 = get_nir_src(instr->src[1]);
> fs_reg data2;
> if (op == BRW_AOP_CMPWR)
>data2 = get_nir_src(instr->src[2]);
>
> -   /* Emit the actual atomic operation operation */
> +   /* Get the offset */
> +   nir_const_value *const_offset = nir_src_as_const_value(instr->src[0]);
> +   if (const_offset) {
> +  offset = brw_imm_ud(instr->const_index[0] + const_offset->u32[0]);
> +   } else {
> +  offset = vgrf(glsl_type::uint_type);
> +  bld.ADD(offset,
> + retype(get_nir_src(instr->src[0]), BRW_REGISTER_TYPE_UD),
> + brw_imm_ud(instr->const_index[0]));
> +   }
> +
> +   /* Emit the actua atomic operation operation */

An l got lost...

>
> fs_reg atomic_result = emit_untyped_atomic(bld, surface, offset,
>data1, data2,
> --
> 2.9.0
>
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH 2/2] i965: Fix shared atomic intrinsics to pay attention to base.

[Kenneth Graunke]

Signed-off-by: Kenneth Graunke 
---
 src/mesa/drivers/dri/i965/brw_fs_nir.cpp | 15 +--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp 
b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
index 6265dc6..a39c37e 100644
--- a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
+++ b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
@@ -4177,13 +4177,24 @@ fs_visitor::nir_emit_shared_atomic(const fs_builder 
,
   dest = get_nir_dest(instr->dest);
 
fs_reg surface = brw_imm_ud(GEN7_BTI_SLM);
-   fs_reg offset = get_nir_src(instr->src[0]);
+   fs_reg offset;
fs_reg data1 = get_nir_src(instr->src[1]);
fs_reg data2;
if (op == BRW_AOP_CMPWR)
   data2 = get_nir_src(instr->src[2]);
 
-   /* Emit the actual atomic operation operation */
+   /* Get the offset */
+   nir_const_value *const_offset = nir_src_as_const_value(instr->src[0]);
+   if (const_offset) {
+  offset = brw_imm_ud(instr->const_index[0] + const_offset->u32[0]);
+   } else {
+  offset = vgrf(glsl_type::uint_type);
+  bld.ADD(offset,
+ retype(get_nir_src(instr->src[0]), BRW_REGISTER_TYPE_UD),
+ brw_imm_ud(instr->const_index[0]));
+   }
+
+   /* Emit the actua atomic operation operation */
 
fs_reg atomic_result = emit_untyped_atomic(bld, surface, offset,
   data1, data2,
-- 
2.9.0

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