[Mesa-stable] [Mesa-dev] [PATCH] nir: Destination component count of shader_clock intrinsic is 2

[Boyan Ding]

2017-04-13 2:25 GMT+08:00 Jason Ekstrand <jason at jlekstrand.net>:
> On Wed, Apr 12, 2017 at 6:14 AM, Boyan Ding <boyan.j.ding at gmail.com> wrote:
>>
>> This fixes the following error when using ARB_shader_clock on i965:
>>         vec1 32 ssa_0 = intrinsic shader_clock () () ()
>>         intrinsic store_var (ssa_0) (clock_retval) (3) /* wrmask=xy */
>> error: src->ssa->num_components == num_components (nir/nir_validate.c:204)
>>
>> Cc: mesa-stable at lists.freedesktop.org
>> Signed-off-by: Boyan Ding <boyan.j.ding at gmail.com>
>> ---
>>  src/compiler/glsl/glsl_to_nir.cpp | 3 ++-
>>  src/compiler/nir/nir_intrinsics.h | 2 +-
>>  2 files changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/compiler/glsl/glsl_to_nir.cpp
>> b/src/compiler/glsl/glsl_to_nir.cpp
>> index f0557f985b..870d457681 100644
>> --- a/src/compiler/glsl/glsl_to_nir.cpp
>> +++ b/src/compiler/glsl/glsl_to_nir.cpp
>> @@ -930,7 +930,8 @@ nir_visitor::visit(ir_call *ir)
>>           nir_builder_instr_insert(&b, &instr->instr);
>>           break;
>>        case nir_intrinsic_shader_clock:
>> -         nir_ssa_dest_init(&instr->instr, &instr->dest, 1, 32, NULL);
>> +         nir_ssa_dest_init(&instr->instr, &instr->dest, 2, 32, NULL);
>> +         instr->num_components = 2;
>
>
> This made me go look at the spec, and things get a bit more subtle...  In
> particular, ARB_shader_clock specifies two builtin functions:
>
>         uvec2 clock2x32ARB(void);
>         uint64_t clockARB(void);
>
>  Where the second one only exists if you support int64.  On gen8+, we do
> support int64...
>
> My feeling is that the correct way to implement this is to make the NIR
> intrinsic return a 64bit value and wrap it in a nir_unpack_64_2x32 if the
> client asks for the 2x32 version.  If that's too much refactoring for you,
> then this patch is probably sufficient to solve the issue today.
>

I agree with you. I'm not very familiar with nir internals, and was
just copying TGSI's handling here. There will be more intrinsics with
64bit results, for example, ballot, which radv guys might be
interested in.

I won't mind if someone comes up with a better solution and replaces
mine. But just as you said above, it solves the issue today. It's up
to you to decide.

Cheers,
Boyan Ding

>>           nir_builder_instr_insert(&b, &instr->instr);
>>           break;
>>        case nir_intrinsic_store_ssbo: {
>> diff --git a/src/compiler/nir/nir_intrinsics.h
>> b/src/compiler/nir/nir_intrinsics.h
>> index 105c56f759..3a519a73dd 100644
>> --- a/src/compiler/nir/nir_intrinsics.h
>> +++ b/src/compiler/nir/nir_intrinsics.h
>> @@ -91,7 +91,7 @@ BARRIER(memory_barrier)
>>   * The latter can be used as code motion barrier, which is currently not
>>   * feasible with NIR.
>>   */
>> -INTRINSIC(shader_clock, 0, ARR(0), true, 1, 0, 0, xx, xx, xx,
>> NIR_INTRINSIC_CAN_ELIMINATE)
>> +INTRINSIC(shader_clock, 0, ARR(0), true, 2, 0, 0, xx, xx, xx,
>> NIR_INTRINSIC_CAN_ELIMINATE)
>>
>>  /*
>>   * Memory barrier with semantics analogous to the compute shader
>> --
>> 2.12.0
>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
>