[Mesa-dev] [PATCH v3 3/5] nir: convert the glsl intrinsic image_size to nir_intrinsic_image_size

[Martin Peres]

On 19/08/15 16:47, Francisco Jerez wrote:
> Martin Peres <martin.peres at linux.intel.com> writes:
>
>> v2, review from Francisco Jerez:
>>   - make the destination variable as large as what the nir instrinsic
>>     defines (4) instead of the size of the return variable of glsl. This
>>     is still safe for the already existing code because all the intrinsics
>>     affected returned the same amount of components as expected by glsl IR.
>>     In the case of image_size, it is not possible to do so because the
>>     returned number of component depends on the image type and this case
>>     is not well handled by nir.
>>
>> Signed-off-by: Martin Peres <martin.peres at linux.intel.com>
>> ---
>>   src/glsl/nir/glsl_to_nir.cpp  | 21 +++++++++++++++------
>>   src/glsl/nir/nir_intrinsics.h |  2 ++
>>   2 files changed, 17 insertions(+), 6 deletions(-)
>>
>> diff --git a/src/glsl/nir/glsl_to_nir.cpp b/src/glsl/nir/glsl_to_nir.cpp
>> index 77327b6..3063243 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_image_size") == 0) {
>> +         op = nir_intrinsic_image_size;
>>         } else {
>>            unreachable("not reached");
>>         }
>> @@ -666,7 +668,8 @@ nir_visitor::visit(ir_call *ir)
>>         case nir_intrinsic_image_atomic_or:
>>         case nir_intrinsic_image_atomic_xor:
>>         case nir_intrinsic_image_atomic_exchange:
>> -      case nir_intrinsic_image_atomic_comp_swap: {
>> +      case nir_intrinsic_image_atomic_comp_swap:
>> +      case nir_intrinsic_image_size: {
>>            nir_ssa_undef_instr *instr_undef =
>>               nir_ssa_undef_instr_create(shader, 1);
>>            nir_instr_insert_after_cf_list(this->cf_node_list,
>> @@ -681,6 +684,17 @@ nir_visitor::visit(ir_call *ir)
>>            instr->variables[0] = evaluate_deref(&instr->instr, image);
>>            param = param->get_next();
>>   
>> +         /* Set the intrinsic destination. */
>> +         if (ir->return_deref) {
>> +            const nir_intrinsic_info *info;
>> +            info = &nir_intrinsic_infos[instr->intrinsic];
> I'm a fan of initializing things at the same point where they are
> defined.  You only use it once though, you may want to drop the
> declaration altogether.

It exceeded the 80-char limit. I guess I can put the assignation on the 
following line.

>
>> +            nir_ssa_dest_init(&instr->instr, &instr->dest,
>> +                              info->dest_components, NULL);
>> +         }
>> +
>> +         if (op == nir_intrinsic_image_size)
>> +            break;
>> +
>>            /* Set the address argument, extending the coordinate vector to four
>>             * components.
>>             */
>> @@ -721,11 +735,6 @@ nir_visitor::visit(ir_call *ir)
>>               instr->src[3] = evaluate_rvalue((ir_dereference *)param);
>>               param = param->get_next();
>>            }
>> -
>> -         /* Set the intrinsic destination. */
>> -         if (ir->return_deref)
>> -            nir_ssa_dest_init(&instr->instr, &instr->dest,
>> -                              ir->return_deref->type->vector_elements, NULL);
>>            break;
>>         }
>>         case nir_intrinsic_memory_barrier:
>> diff --git a/src/glsl/nir/nir_intrinsics.h b/src/glsl/nir/nir_intrinsics.h
>> index bc6e6b8..6c7a61a 100644
>> --- a/src/glsl/nir/nir_intrinsics.h
>> +++ b/src/glsl/nir/nir_intrinsics.h
>> @@ -123,6 +123,8 @@ INTRINSIC(image_atomic_or, 3, ARR(4, 1, 1), true, 1, 1, 0, 0)
>>   INTRINSIC(image_atomic_xor, 3, ARR(4, 1, 1), true, 1, 1, 0, 0)
>>   INTRINSIC(image_atomic_exchange, 3, ARR(4, 1, 1), true, 1, 1, 0, 0)
>>   INTRINSIC(image_atomic_comp_swap, 4, ARR(4, 1, 1, 1), true, 1, 1, 0, 0)
>> +INTRINSIC(image_size, 0, ARR(), true, 4, 1, 0,
>> +          NIR_INTRINSIC_CAN_ELIMINATE | NIR_INTRINSIC_CAN_REORDER)
>>   
> Looks good to me,
> Reviewed-by: Francisco Jerez <currojerez at riseup.net>

Thanks for the review, I will address the comments on the previous patch!

>
>>   #define SYSTEM_VALUE(name, components) \
>>      INTRINSIC(load_##name, 0, ARR(), true, components, 0, 0, \
>> -- 
>> 2.5.0
>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev