Re: [Mesa-dev] [PATCH 2/2] i965: Fix shared atomic intrinsics to pay attention to base.
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 &bld, >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.
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.
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 &bld, > 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.
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 &bld, > 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.
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 > &bld, >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.
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 &bld, 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