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.
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
