On 22/09/15 20:57, Kristian Høgsberg wrote:
> 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.
>
OK, good idea. I am going to work on cleaning up that code.
Thanks,
Sam
> 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
