On Mon, Sep 21, 2015 at 11:03:41AM +0200, Samuel Iglesias Gonsálvez wrote:
>
>
> On 19/09/15 00:30, Kristian Høgsberg wrote:
> > On Thu, Sep 10, 2015 at 03:35:56PM +0200, Iago Toral Quiroga wrote:
> >> v2:
> >> - Fix ssbo loads with boolean variables.
> >>
> >> Reviewed-by: Connor Abbott <connor.w.abb...@intel.com>
> >> ---
> >> src/glsl/nir/glsl_to_nir.cpp | 80
> >> ++++++++++++++++++++++++++++++++-
> >> src/glsl/nir/nir_intrinsics.h | 2 +-
> >> src/glsl/nir/nir_lower_phis_to_scalar.c | 2 +
> >> 3 files changed, 81 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/src/glsl/nir/glsl_to_nir.cpp b/src/glsl/nir/glsl_to_nir.cpp
> >> index 6f1e20a..cb7b196 100644
> >> --- a/src/glsl/nir/glsl_to_nir.cpp
> >> +++ b/src/glsl/nir/glsl_to_nir.cpp
> >> @@ -646,11 +646,14 @@ nir_visitor::visit(ir_call *ir)
> >> op = nir_intrinsic_image_size;
> >> } else if (strcmp(ir->callee_name(), "__intrinsic_store_ssbo") ==
> >> 0) {
> >> op = nir_intrinsic_store_ssbo;
> >> + } else if (strcmp(ir->callee_name(), "__intrinsic_load_ssbo") == 0)
> >> {
> >> + op = nir_intrinsic_load_ssbo;
> >> } else {
> >> unreachable("not reached");
> >> }
> >>
> >> nir_intrinsic_instr *instr = nir_intrinsic_instr_create(shader, op);
> >> + nir_alu_instr *load_ssbo_compare;
> >>
> >> switch (op) {
> >> case nir_intrinsic_atomic_counter_read_var:
> >> @@ -776,11 +779,75 @@ nir_visitor::visit(ir_call *ir)
> >> instr->src[1] = evaluate_rvalue(block);
> >> break;
> >> }
> >> + case nir_intrinsic_load_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();
> >> +
> >> + /* Check if we need the indirect version */
> >> + ir_constant *const_offset = offset->as_constant();
> >> + if (!const_offset) {
> >> + op = nir_intrinsic_load_ssbo_indirect;
> >> + ralloc_free(instr);
> >> + instr = nir_intrinsic_instr_create(shader, op);
> >> + instr->src[1] = evaluate_rvalue(offset);
> >> + instr->const_index[0] = 0;
> >> + } else {
> >> + instr->const_index[0] = const_offset->value.u[0];
> >> + }
> >> +
> >> + instr->src[0] = evaluate_rvalue(block);
> >> +
> >> + const glsl_type *type = ir->return_deref->var->type;
> >> + instr->num_components = type->vector_elements;
> >> +
> >> + /* Setup destination register */
> >> + nir_ssa_dest_init(&instr->instr, &instr->dest,
> >> + type->vector_elements, NULL);
> >> +
> >> + /* Insert the created nir instruction now since in the case of
> >> boolean
> >> + * result we will need to emit another instruction after it
> >> + */
> >> + nir_instr_insert_after_cf_list(this->cf_node_list,
> >> &instr->instr);
> >
> > I'd prefer moving this insert into the GLSL_TYPE_BOOL condition below...
> >
> >> + /*
> >> + * In SSBO/UBO's, a true boolean value is any non-zero value,
> >> but we
> >> + * consider a true boolean to be ~0. Fix this up with a != 0
> >> + * comparison.
> >> + */
> >> + if (type->base_type == GLSL_TYPE_BOOL) {
> >
> > ... that is, here...
> >
> >> + nir_load_const_instr *const_zero =
> >> + nir_load_const_instr_create(shader, 1);
> >> + const_zero->value.u[0] = 0;
> >> + nir_instr_insert_after_cf_list(this->cf_node_list,
> >> + &const_zero->instr);
> >> +
> >> + load_ssbo_compare = nir_alu_instr_create(shader, nir_op_ine);
> >
> > and then, insteed of introducing and using 'load_ssbo_compare' here,
> > re-use 'instr' for the compare instruction...
> >
>
> I see why you suggest these changes but I don't like them:
>
> instr is a pointer to nir_instrinsic_instr, while the compare
> instruction is a nir_alu_instr*, so although we are lucky because some
> fields are at the same offsets for both struct base addresses(*), this
> could change in the future.
Right, they're different types... it's still unfortunate that the ssbo
special case spills out of the nir_intrinsic_load_ssbo case in the
switch statement.
Instead of jumping through hoops to share the one
nir_instr_insert_after_cf_list call between the switch cases, maybe
it's more straightforward to move that call into each case. That way
the code can explicitly decide how order the intrinsic call and other
helper instructions. Then set a nir_dest pointer for the
ir->return_deref handling after the switch. We can initialize the
nir_dest pointers to &instr->dest before the switch for the default
case.
Kristian
> Even if I introduce load_ssbo_compare and, after setting it up, assign
> 'instr' with load_ssbo_compare pointer value to save some changes done
> later, this would work now but it is not guaranteed to work in the future.
>
> Sam
>
> (*) For example:
> load_ssbo_compare->dest.dest.ssa and instr->dest.ssa
>
> >> + load_ssbo_compare->src[0].src.is_ssa = true;
> >> + load_ssbo_compare->src[0].src.ssa = &instr->dest.ssa;
> >> + load_ssbo_compare->src[1].src.is_ssa = true;
> >> + load_ssbo_compare->src[1].src.ssa = &const_zero->def;
> >> + for (unsigned i = 0; i < type->vector_elements; i++)
> >> + load_ssbo_compare->src[1].swizzle[i] = 0;
> >> + nir_ssa_dest_init(&load_ssbo_compare->instr,
> >> + &load_ssbo_compare->dest.dest,
> >> + type->vector_elements, NULL);
> >> + load_ssbo_compare->dest.write_mask = (1 <<
> >> type->vector_elements) - 1;
> >> + nir_instr_insert_after_cf_list(this->cf_node_list,
> >> + &load_ssbo_compare->instr);
> >> + }
> >> +
> >> + break;
> >> + }
> >> default:
> >> unreachable("not reached");
> >> }
> >>
> >> - nir_instr_insert_after_cf_list(this->cf_node_list, &instr->instr);
> >> + /* nir_intrinsic_load_ssbo{_indirect} were already emitted */
> >> + if (op != nir_intrinsic_load_ssbo && op !=
> >> nir_intrinsic_load_ssbo_indirect)
> >> + nir_instr_insert_after_cf_list(this->cf_node_list,
> >> &instr->instr);
> >>
> >
> > That way we don't need anything special here for ssbo_load...
> >
> >> if (ir->return_deref) {
> >> nir_intrinsic_instr *store_instr =
> >> @@ -789,7 +856,16 @@ nir_visitor::visit(ir_call *ir)
> >>
> >> store_instr->variables[0] =
> >> evaluate_deref(&store_instr->instr, ir->return_deref);
> >> - store_instr->src[0] = nir_src_for_ssa(&instr->dest.ssa);
> >> +
> >> + /* If nir_intrinsic_load_ssbo{_indirect} is loading a boolean
> >> variable,
> >> + * the value is on load_ssbo_compare's dest. Use it instead.
> >> + */
> >> + if ((op == nir_intrinsic_load_ssbo || op ==
> >> nir_intrinsic_load_ssbo_indirect) &&
> >> + ir->return_deref->var->type->base_type == GLSL_TYPE_BOOL) {
> >> + store_instr->src[0] =
> >> nir_src_for_ssa(&load_ssbo_compare->dest.dest.ssa);
> >> + } else {
> >> + store_instr->src[0] = nir_src_for_ssa(&instr->dest.ssa);
> >> + }
> >
> > nor here.
> >
> >> nir_instr_insert_after_cf_list(this->cf_node_list,
> >> &store_instr->instr);
> >> diff --git a/src/glsl/nir/nir_intrinsics.h b/src/glsl/nir/nir_intrinsics.h
> >> index 38f22c1..53066c6 100644
> >> --- a/src/glsl/nir/nir_intrinsics.h
> >> +++ b/src/glsl/nir/nir_intrinsics.h
> >> @@ -174,7 +174,7 @@ SYSTEM_VALUE(invocation_id, 1)
> >> LOAD(uniform, 0, 2, NIR_INTRINSIC_CAN_ELIMINATE |
> >> NIR_INTRINSIC_CAN_REORDER)
> >> LOAD(ubo, 1, 1, NIR_INTRINSIC_CAN_ELIMINATE | NIR_INTRINSIC_CAN_REORDER)
> >> LOAD(input, 0, 1, NIR_INTRINSIC_CAN_ELIMINATE | NIR_INTRINSIC_CAN_REORDER)
> >> -/* LOAD(ssbo, 1, 0) */
> >> +LOAD(ssbo, 1, 1, NIR_INTRINSIC_CAN_ELIMINATE)
> >>
> >> /*
> >> * Stores work the same way as loads, except now the first register input
> >> is
> >> diff --git a/src/glsl/nir/nir_lower_phis_to_scalar.c
> >> b/src/glsl/nir/nir_lower_phis_to_scalar.c
> >> index 739170d..bdb7e6a 100644
> >> --- a/src/glsl/nir/nir_lower_phis_to_scalar.c
> >> +++ b/src/glsl/nir/nir_lower_phis_to_scalar.c
> >> @@ -94,6 +94,8 @@ is_phi_src_scalarizable(nir_phi_src *src,
> >> case nir_intrinsic_load_uniform_indirect:
> >> case nir_intrinsic_load_ubo:
> >> case nir_intrinsic_load_ubo_indirect:
> >> + case nir_intrinsic_load_ssbo:
> >> + case nir_intrinsic_load_ssbo_indirect:
> >> case nir_intrinsic_load_input:
> >> case nir_intrinsic_load_input_indirect:
> >> return true;
> >> --
> >> 1.9.1
> >>
> >
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
