[virglrenderer-devel] [PATCH 3/4] shader: add basic shader_storage_buffer_object parsing. (v2)

[Dave Airlie]

On 18 July 2018 at 21:10, Tomeu Vizoso <tomeu.vizoso at collabora.com> wrote:
> On 07/18/2018 04:26 AM, Dave Airlie wrote:
>>
>> From: Dave Airlie <airlied at redhat.com>
>>
>> This adds the basic shader parsing for the SSBO extension,
>>
>> v2: drop qualifier, cleanup unused var, add indirect support,
>> use a bitmask to track declared ssbos.
>> ---
>>   src/vrend_shader.c | 331
>> ++++++++++++++++++++++++++++++++++++++++++++++++-----
>>   src/vrend_shader.h |   1 +
>>   2 files changed, 305 insertions(+), 27 deletions(-)
>>
>> diff --git a/src/vrend_shader.c b/src/vrend_shader.c
>> index cbbfbbc..b20d681 100644
>> --- a/src/vrend_shader.c
>> +++ b/src/vrend_shader.c
>> @@ -108,6 +108,28 @@ struct vrend_io_range {
>>      bool used;
>>   };
>>   +enum vrend_type_qualifier {
>
>
> Apparently this enum doesn't need to be moved around?
>
>
>> +   TYPE_CONVERSION_NONE = 0,
>> +   FLOAT = 1,
>> +   VEC2 = 2,
>> +   VEC3 = 3,
>> +   VEC4 = 4,
>> +   INT = 5,
>> +   IVEC2 = 6,
>> +   IVEC3 = 7,
>> +   IVEC4 = 8,
>> +   UINT = 9,
>> +   UVEC2 = 10,
>> +   UVEC3 = 11,
>> +   UVEC4 = 12,
>> +   FLOAT_BITS_TO_UINT = 13,
>> +   UINT_BITS_TO_FLOAT = 14,
>> +   FLOAT_BITS_TO_INT = 15,
>> +   INT_BITS_TO_FLOAT = 16,
>> +   DOUBLE = 17,
>> +   DVEC2 = 18,
>> +};
>> +
>>   struct dump_ctx {
>>      struct tgsi_iterate_context iter;
>>      struct vrend_shader_cfg *cfg;
>> @@ -136,7 +158,12 @@ struct dump_ctx {
>>        struct vrend_shader_sampler samplers[32];
>>      uint32_t samplers_used;
>> -   bool sviews_used;
>> +
>> +   uint32_t ssbo_used_mask;
>> +   uint32_t ssbo_atomic_mask;
>> +   uint32_t ssbo_array_base;
>> +   uint32_t ssbo_atomic_array_base;
>> +
>>      struct vrend_sampler_array *sampler_arrays;
>>      uint32_t num_sampler_arrays;
>>      int last_sampler_array_idx;
>> @@ -206,28 +233,6 @@ static const struct vrend_shader_table
>> shader_req_table[] = {
>>       { SHADER_REQ_FP64, "GL_ARB_gpu_shader_fp64" },
>>   };
>>   -enum vrend_type_qualifier {
>> -   TYPE_CONVERSION_NONE = 0,
>> -   FLOAT = 1,
>> -   VEC2 = 2,
>> -   VEC3 = 3,
>> -   VEC4 = 4,
>> -   INT = 5,
>> -   IVEC2 = 6,
>> -   IVEC3 = 7,
>> -   IVEC4 = 8,
>> -   UINT = 9,
>> -   UVEC2 = 10,
>> -   UVEC3 = 11,
>> -   UVEC4 = 12,
>> -   FLOAT_BITS_TO_UINT = 13,
>> -   UINT_BITS_TO_FLOAT = 14,
>> -   FLOAT_BITS_TO_INT = 15,
>> -   INT_BITS_TO_FLOAT = 16,
>> -   DOUBLE = 17,
>> -   DVEC2 = 18,
>> -};
>> -
>>   struct dest_info {
>>     enum vrend_type_qualifier dtypeprefix;
>>     enum vrend_type_qualifier dstconv;
>> @@ -1040,8 +1045,22 @@ iter_declaration(struct tgsi_iterate_context *iter,
>>            } else {
>>               ctx->last_sampler_array_idx = add_sampler_array(ctx,
>> decl->Range.First, decl->Range.Last + 1, decl->SamplerView.Resource,
>> decl->SamplerView.ReturnTypeX);
>>            }
>> -      } else
>> -      ctx->sviews_used = true;
>
>
> Was this removal intentional?

It was but no for this patch, not sure how I snuck it in here, must
have rebased wrong.

Will resend with it split out.
> conversion is used unconditionally, so this comment doesn't make sense here.
>
>> +      const char *conversion = get_string(FLOAT_BITS_TO_UINT);
>> +      if (inst->Dst[0].Register.WriteMask & 0x1) {
>> +         snprintf(buf, 255, "%s[uint(floatBitsToUint(%s))>>2] =
>> %s(%s).x;\n", dsts[0], srcs[0], conversion, srcs[1]);
>
>
> s/floatBitsToUint/conversion as we already have it?

I was wondering whether to leave conversion in at all at this stage, since
we only set it but we have a future reason to need it so I left it,

However if we use it in the future we only need it where it's used not
on the floatBitsToUint here which is always going to be a uint;

>> +   }
>> +
>> +   EMIT_BUF_WITH_RET(ctx, buf);
>
>
> Guess we don't want to emit an uninitialized buffer?

Good point.

Dave.