[virglrenderer-devel] [PATCH 2/7] shader: add image support to shader parsing. (v2)

[Dave Airlie]

On 20 July 2018 at 22:44, Tomeu Vizoso <tomeu.vizoso at collabora.com> wrote:
> On 07/20/2018 06:50 AM, Dave Airlie wrote:
>>
>> @@ -1047,6 +1087,14 @@ iter_declaration(struct tgsi_iterate_context *iter,
>>            }
>>         }
>>         break;
>> +   case TGSI_FILE_IMAGE:
>> +      ctx->shader_req_bits |= SHADER_REQ_IMAGE_LOAD_STORE;
>> +      if (decl->Range.First >= ARRAY_SIZE(ctx->images)) {
>
>
> Shouldn't this be decl->Range.Last >= ARRAY_SIZE(ctx->images)?

Indeed that seems like it would be more sane!

>
>> @@ -2142,6 +2190,36 @@ create_swizzled_clipdist(struct dump_ctx *ctx,
>>      snprintf(result, 255, "%s(vec4(%s,%s,%s,%s))", stypeprefix,
>> clipdistvec[0], clipdistvec[1], clipdistvec[2], clipdistvec[3]);
>>   }
>>   +static enum vrend_type_qualifier get_coord_prefix(int resource, bool
>> *is_ms)
>> +{
>> +   switch(resource) {
>> +   case TGSI_TEXTURE_1D:
>> +   case TGSI_TEXTURE_BUFFER:
>> +      return INT;
>> +      break;
>
>
> This and the other breaks are superfluous.

I'll drop them.

>
>> @@ -4113,6 +4421,56 @@ static char *emit_ios(struct dump_ctx *ctx, char
>> *glsl_hdr)
>>         }
>>      }
>>   +   if (ctx->info.indirect_files & (1 << TGSI_FILE_IMAGE)) {
>> +      uint32_t mask = ctx->images_used_mask;
>> +      while (mask) {
>> +         int is_shad = 0;
>> +         const char *stc;
>> +         char ptc;
>> +         i = u_bit_scan(&mask);
>> +
>> +         unsigned array_size = 0;
>> +         for (uint32_t j = i; j < util_last_bit(ctx->images_used_mask);
>> j++) {
>> +            if (!(ctx->images_used_mask & (1 << j)))
>> +               break;
>> +            if (!memcmp(&ctx->images[i].decl, &ctx->images[j].decl,
>> sizeof(ctx->images[j].decl)) &&
>> +                ctx->images[i].image_return ==
>> ctx->images[j].image_return) {
>> +               mask &= ~(1 << j);
>> +               array_size++;
>> +            }
>> +         }
>> +         const char *volatile_str = (ctx->images[i].vflag) ? "volatile "
>> : "";
>
>
> I think s/vflag/volatile would be a bit more readable.

volatile is a C reserved keyword unfortunately.

Dave.