Re: [Mesa-dev] [PATCH 01/26] nir: Add a writemask to store intrinsics.
On Wed, Dec 2, 2015 at 4:15 PM, Kenneth Graunke wrote: > Tessellation control shaders need to be careful when writing outputs. > Because multiple threads can concurrently write the same output > variables, we need to only write the exact components we were told. > > Traditionally, for sub-vector writes, we've read the whole vector, > updated the temporary, and written the whole vector back. This breaks > down with concurrent access. > > This patch prepares the way for a solution by adding a writemask field > to store_var intrinsics, as well as the other store intrinsics. It then > updates all produces to emit a writemask of "all channels enabled". It > updates nir_lower_io to copy the writemask to output store intrinsics. > > Finally, it updates nir_lower_vars_to_ssa to handle partial writemasks > by doing a read-modify-write cycle (which is safe, because local > variables are specific to a single thread). > > This should have no functional change, since no one actually emits > partial writemasks yet. > > Signed-off-by: Kenneth Graunke > --- > src/gallium/auxiliary/nir/tgsi_to_nir.c| 1 + > .../drivers/freedreno/ir3/ir3_compiler_nir.c | 6 +++ > src/glsl/nir/glsl_to_nir.cpp | 2 + > src/glsl/nir/nir_builder.h | 4 +- > src/glsl/nir/nir_intrinsics.h | 14 +++ > src/glsl/nir/nir_lower_gs_intrinsics.c | 3 +- > src/glsl/nir/nir_lower_io.c| 2 + > src/glsl/nir/nir_lower_locals_to_regs.c| 2 +- > src/glsl/nir/nir_lower_var_copies.c| 1 + > src/glsl/nir/nir_lower_vars_to_ssa.c | 46 > -- > src/glsl/nir/nir_validate.c| 1 + > src/mesa/program/prog_to_nir.c | 2 + > 12 files changed, 63 insertions(+), 21 deletions(-) > > Technically, I don't think I need the ir3 changes here - it should work > without any changes. I think I was just overzealously preparing things > to handle writemasks before I realized there shouldn't be any. > > But, it might be worth doing anyway. Not sure. > > Jason and I have already talked about the fact that we're conflicting. > I like what he's doing, and he suggested this; we just need to figure out > what lands first. I decided to send these out anyway for people to look > at and start reviewing, since there's a lot to do there. We'll sort it > out in v2. > > diff --git a/src/gallium/auxiliary/nir/tgsi_to_nir.c > b/src/gallium/auxiliary/nir/tgsi_to_nir.c > index 86c2ffa..f5547c8 100644 > --- a/src/gallium/auxiliary/nir/tgsi_to_nir.c > +++ b/src/gallium/auxiliary/nir/tgsi_to_nir.c > @@ -1894,6 +1894,7 @@ ttn_emit_instruction(struct ttn_compile *c) > &tgsi_dst->Indirect : NULL; > >store->num_components = 4; > + store->const_index[0] = 0xf; >store->variables[0] = ttn_array_deref(c, store, var, offset, indirect); >store->src[0] = nir_src_for_reg(dest.dest.reg.reg); > > diff --git a/src/gallium/drivers/freedreno/ir3/ir3_compiler_nir.c > b/src/gallium/drivers/freedreno/ir3/ir3_compiler_nir.c > index 8617704..3beed0e 100644 > --- a/src/gallium/drivers/freedreno/ir3/ir3_compiler_nir.c > +++ b/src/gallium/drivers/freedreno/ir3/ir3_compiler_nir.c > @@ -1314,6 +1314,9 @@ emit_intrinisic_store_var(struct ir3_compile *ctx, > nir_intrinsic_instr *intr) > case nir_deref_array_type_direct: > /* direct access does not require anything special: */ > for (int i = 0; i < intr->num_components; i++) { > + if (!(intr->const_index[0] & (1 << i))) > + continue; > + > unsigned n = darr->base_offset * 4 + i; > compile_assert(ctx, n < arr->length); > arr->arr[n] = src[i]; > @@ -1326,6 +1329,9 @@ emit_intrinisic_store_var(struct ir3_compile *ctx, > nir_intrinsic_instr *intr) > struct ir3_instruction *addr = > get_addr(ctx, get_src(ctx, > &darr->indirect)[0]); > for (int i = 0; i < intr->num_components; i++) { > + if (!(intr->const_index[0] & (1 << i))) > + continue; > + > struct ir3_instruction *store; > unsigned n = darr->base_offset * 4 + i; > compile_assert(ctx, n < arr->length); > diff --git a/src/glsl/nir/glsl_to_nir.cpp b/src/glsl/nir/glsl_to_nir.cpp > index 45d045c..5c84b0d 100644 > --- a/src/glsl/nir/glsl_to_nir.cpp > +++ b/src/glsl/nir/glsl_to_nir.cpp > @@ -982,6 +982,7 @@ nir_visitor::visit(ir_call *ir) > nir_intrinsic_instr *store_instr = > nir_intrinsic_instr_create(shader, nir_intrinsic_store_var); > store_instr->num_components = > ir->return_deref->type->vector_elements; > + store_instr->cons
Re: [Mesa-dev] [PATCH 01/26] nir: Add a writemask to store intrinsics.
On Friday, December 04, 2015 04:45:03 PM Jason Ekstrand wrote: > On Wed, Dec 2, 2015 at 4:15 PM, Kenneth Graunke wrote: > > Tessellation control shaders need to be careful when writing outputs. > > Because multiple threads can concurrently write the same output > > variables, we need to only write the exact components we were told. > > > > Traditionally, for sub-vector writes, we've read the whole vector, > > updated the temporary, and written the whole vector back. This breaks > > down with concurrent access. > > > > This patch prepares the way for a solution by adding a writemask field > > to store_var intrinsics, as well as the other store intrinsics. It then > > updates all produces to emit a writemask of "all channels enabled". It > > updates nir_lower_io to copy the writemask to output store intrinsics. > > > > Finally, it updates nir_lower_vars_to_ssa to handle partial writemasks > > by doing a read-modify-write cycle (which is safe, because local > > variables are specific to a single thread). > > > > This should have no functional change, since no one actually emits > > partial writemasks yet. > > > > Signed-off-by: Kenneth Graunke > > --- > > src/gallium/auxiliary/nir/tgsi_to_nir.c| 1 + > > .../drivers/freedreno/ir3/ir3_compiler_nir.c | 6 +++ > > src/glsl/nir/glsl_to_nir.cpp | 2 + > > src/glsl/nir/nir_builder.h | 4 +- > > src/glsl/nir/nir_intrinsics.h | 14 +++ > > src/glsl/nir/nir_lower_gs_intrinsics.c | 3 +- > > src/glsl/nir/nir_lower_io.c| 2 + > > src/glsl/nir/nir_lower_locals_to_regs.c| 2 +- > > src/glsl/nir/nir_lower_var_copies.c| 1 + > > src/glsl/nir/nir_lower_vars_to_ssa.c | 46 > > -- > > src/glsl/nir/nir_validate.c| 1 + > > src/mesa/program/prog_to_nir.c | 2 + > > 12 files changed, 63 insertions(+), 21 deletions(-) > > > > Technically, I don't think I need the ir3 changes here - it should work > > without any changes. I think I was just overzealously preparing things > > to handle writemasks before I realized there shouldn't be any. > > > > But, it might be worth doing anyway. Not sure. > > > > Jason and I have already talked about the fact that we're conflicting. > > I like what he's doing, and he suggested this; we just need to figure out > > what lands first. I decided to send these out anyway for people to look > > at and start reviewing, since there's a lot to do there. We'll sort it > > out in v2. > > > > diff --git a/src/gallium/auxiliary/nir/tgsi_to_nir.c > > b/src/gallium/auxiliary/nir/tgsi_to_nir.c > > index 86c2ffa..f5547c8 100644 > > --- a/src/gallium/auxiliary/nir/tgsi_to_nir.c > > +++ b/src/gallium/auxiliary/nir/tgsi_to_nir.c > > @@ -1894,6 +1894,7 @@ ttn_emit_instruction(struct ttn_compile *c) > > &tgsi_dst->Indirect : NULL; > > > >store->num_components = 4; > > + store->const_index[0] = 0xf; > >store->variables[0] = ttn_array_deref(c, store, var, offset, > > indirect); > >store->src[0] = nir_src_for_reg(dest.dest.reg.reg); > > > > diff --git a/src/gallium/drivers/freedreno/ir3/ir3_compiler_nir.c > > b/src/gallium/drivers/freedreno/ir3/ir3_compiler_nir.c > > index 8617704..3beed0e 100644 > > --- a/src/gallium/drivers/freedreno/ir3/ir3_compiler_nir.c > > +++ b/src/gallium/drivers/freedreno/ir3/ir3_compiler_nir.c > > @@ -1314,6 +1314,9 @@ emit_intrinisic_store_var(struct ir3_compile *ctx, > > nir_intrinsic_instr *intr) > > case nir_deref_array_type_direct: > > /* direct access does not require anything special: */ > > for (int i = 0; i < intr->num_components; i++) { > > + if (!(intr->const_index[0] & (1 << i))) > > + continue; > > + > > unsigned n = darr->base_offset * 4 + i; > > compile_assert(ctx, n < arr->length); > > arr->arr[n] = src[i]; > > @@ -1326,6 +1329,9 @@ emit_intrinisic_store_var(struct ir3_compile *ctx, > > nir_intrinsic_instr *intr) > > struct ir3_instruction *addr = > > get_addr(ctx, get_src(ctx, > > &darr->indirect)[0]); > > for (int i = 0; i < intr->num_components; i++) { > > + if (!(intr->const_index[0] & (1 << i))) > > + continue; > > + > > struct ir3_instruction *store; > > unsigned n = darr->base_offset * 4 + i; > > compile_assert(ctx, n < arr->length); > > diff --git a/src/glsl/nir/glsl_to_nir.cpp b/src/glsl/nir/glsl_to_nir.cpp > > index 45d045c..5c84b0d 100644 > > --- a/src/glsl/nir/glsl_to_nir.cpp > > +++ b/src/glsl/nir/glsl_to_nir.cpp > > @@ -982,6 +982,7 @@ nir_visito