On Thu, 2015-08-06 at 11:06 -0700, Connor Abbott wrote: > On Thu, Aug 6, 2015 at 12:30 AM, Iago Toral <ito...@igalia.com> wrote: > > On Wed, 2015-08-05 at 12:17 -0700, Connor Abbott wrote: > >> On Wed, Aug 5, 2015 at 1:30 AM, Iago Toral Quiroga <ito...@igalia.com> > >> wrote: > >> > --- > >> > src/glsl/nir/glsl_to_nir.cpp | 36 ++++++++++++++++++++++++++++++++++++ > >> > src/glsl/nir/nir_intrinsics.h | 12 ++++++------ > >> > 2 files changed, 42 insertions(+), 6 deletions(-) > >> > > >> > diff --git a/src/glsl/nir/glsl_to_nir.cpp b/src/glsl/nir/glsl_to_nir.cpp > >> > index 642affd..cbec2df 100644 > >> > --- a/src/glsl/nir/glsl_to_nir.cpp > >> > +++ b/src/glsl/nir/glsl_to_nir.cpp > >> > @@ -641,6 +641,8 @@ nir_visitor::visit(ir_call *ir) > >> > op = nir_intrinsic_image_atomic_comp_swap; > >> > } else if (strcmp(ir->callee_name(), > >> > "__intrinsic_memory_barrier") == 0) { > >> > op = nir_intrinsic_memory_barrier; > >> > + } else if (strcmp(ir->callee_name(), "__intrinsic_store_ssbo") == > >> > 0) { > >> > + op = nir_intrinsic_store_ssbo; > >> > } else { > >> > unreachable("not reached"); > >> > } > >> > @@ -730,6 +732,40 @@ nir_visitor::visit(ir_call *ir) > >> > } > >> > case nir_intrinsic_memory_barrier: > >> > break; > >> > + case nir_intrinsic_store_ssbo: { > >> > + exec_node *param = ir->actual_parameters.get_head(); > >> > + ir_rvalue *block = ((ir_instruction *)param)->as_rvalue(); > >> > + > >> > + param = param->get_next(); > >> > + ir_rvalue *offset = ((ir_instruction *)param)->as_rvalue(); > >> > + > >> > + param = param->get_next(); > >> > + ir_rvalue *val = ((ir_instruction *)param)->as_rvalue(); > >> > + > >> > + param = param->get_next(); > >> > + ir_constant *write_mask = ((ir_instruction > >> > *)param)->as_constant(); > >> > + assert(write_mask); > >> > + > >> > + /* Check if we need the indirect version */ > >> > + ir_constant *const_offset = offset->as_constant(); > >> > + if (!const_offset) { > >> > + op = nir_intrinsic_store_ssbo_indirect; > >> > + ralloc_free(instr); > >> > + instr = nir_intrinsic_instr_create(shader, op); > >> > + instr->src[2] = evaluate_rvalue(offset); > >> > + instr->const_index[0] = 0; > >> > + } else { > >> > + instr->const_index[0] = const_offset->value.u[0]; > >> > + } > >> > + > >> > + instr->const_index[1] = write_mask->value.u[0]; > >> > + > >> > + instr->src[0] = evaluate_rvalue(val); > >> > + instr->num_components = val->type->vector_elements; > >> > + > >> > + instr->src[1] = evaluate_rvalue(block); > >> > + break; > >> > + } > >> > default: > >> > unreachable("not reached"); > >> > } > >> > diff --git a/src/glsl/nir/nir_intrinsics.h > >> > b/src/glsl/nir/nir_intrinsics.h > >> > index f264f55..83eeecd 100644 > >> > --- a/src/glsl/nir/nir_intrinsics.h > >> > +++ b/src/glsl/nir/nir_intrinsics.h > >> > @@ -176,12 +176,12 @@ LOAD(input, 0, NIR_INTRINSIC_CAN_ELIMINATE | > >> > NIR_INTRINSIC_CAN_REORDER) > >> > * offset. > >> > */ > >> > > >> > -#define STORE(name, num_indices, flags) \ > >> > - INTRINSIC(store_##name, 1, ARR(0), false, 0, 0, num_indices, flags) \ > >> > - INTRINSIC(store_##name##_indirect, 2, ARR(0, 1), false, 0, 0, \ > >> > +#define STORE(name, extra_srcs, num_indices, flags) \ > >> > + INTRINSIC(store_##name, extra_srcs, ARR(0, 1), false, 0, 0, > >> > num_indices, flags) \ > >> > + INTRINSIC(store_##name##_indirect, extra_srcs + 1, ARR(0, 1, 1), > >> > false, 0, 0, \ > >> > num_indices, flags) \ > >> > > >> > -STORE(output, 1, 0) > >> > -/* STORE(ssbo, 2, 0) */ > >> > +STORE(output, 1, 2, 0) > >> > +STORE(ssbo, 2, 2, 0) > >> > >> I don't think outputs should have any extra sources, since they only > >> take a constant index, plus possibly an indirect source that's already > >> covered by the STORE macro. SSBO stores should only have one extra > >> source for the block index. Also, we should update the comment above > >> to explain this similarly to the paragraph above the loads. > > > > SSBO stores need an extra source for the block index and an extra index > > for a writemask. > > > > I'll leave the STORE() macro as it was and just define SSBO stores using > > INTRINSIC() directly then. > > Ok, I see. I don't think you need a separate INTRINSIC(), but right > now calling the parameter you added "extra_srcs" is confusing, since > you're counting the value to be stored, which isn't really "extra" at > all -- every store should have one! How about instead, we change the > STORE macro to have: > > - An "extra_srcs" parameter that contains only sources that are > actually extra, not counting the value to be stored -- direct stores > have "extra_srcs + 1" sources, and indirect sources have "extra_srcs + > 2" sources > - An "extra_indices" parameter that contains the extra indices, and > replace "num_indices" with "extra_indices + 1" > > Then normal stores have both set to 0, and SSBO stores have both set > to 1 to indicate the extra block index and writemask.
Sure, sounds good to me. Iago > > > >> > > >> > -LAST_INTRINSIC(store_output_indirect) > >> > +LAST_INTRINSIC(store_ssbo_indirect) > >> > -- > >> > 1.9.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