[Mesa-dev] [PATCH 1/2] glsl: store the image format in glsl_struct_field

[Nicolai Hähnle]

On 06.05.2017 20:09, Jason Ekstrand wrote:
> On May 6, 2017 7:56:02 AM Samuel Pitoiset <samuel.pitoiset at gmail.com>
> wrote:
>
>> ARB_bindless_texture allows to declare image types inside
>> structures,
>
> Of course it does... It's not like having samplers and uniforms in
> structures is a massive source of pain or anything like that...
>
>> which means we need to keep track of the format.
>
> Right.  Does it make more sense to put this on the structure element or
> on the image type itself?  SPIR-V makes the format part of the image
> type.  Doing so would also let us potentially get rid of the field in
> [n]ir_variable.

 From a type-theory point of view, I'd say yes.

On the other hand, this leads to a massive explosion in the number of 
types, which doesn't play well with how GLSL overloads work. I really 
don't think we should pre-generate all the possible type combinations 
(probably > 1000 types for every texture target) *and* have duplicate 
intrinsic signatures for all of them.

Long-term, moving to having the format (and other qualifiers!) part of 
the type is a good idea. But before doing that, we'd have to seriously 
restructure how GLSL overloads work.

In the meantime, these two patches look reasonable to me:

Reviewed-by: Nicolai Hähnle <nicolai.haehnle at amd.com>

Cheers,
Nicolai


>
>> Signed-off-by: Samuel Pitoiset <samuel.pitoiset at gmail.com>
>> ---
>>  src/compiler/glsl/builtin_variables.cpp | 1 +
>>  src/compiler/glsl_types.cpp             | 3 +++
>>  src/compiler/glsl_types.h               | 8 +++++++-
>>  3 files changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/compiler/glsl/builtin_variables.cpp
>> b/src/compiler/glsl/builtin_variables.cpp
>> index a45c9d62c7..ce4dd43730 100644
>> --- a/src/compiler/glsl/builtin_variables.cpp
>> +++ b/src/compiler/glsl/builtin_variables.cpp
>> @@ -341,6 +341,7 @@ per_vertex_accumulator::add_field(int slot, const
>> glsl_type *type,
>>     this->fields[this->num_fields].memory_coherent = 0;
>>     this->fields[this->num_fields].memory_volatile = 0;
>>     this->fields[this->num_fields].memory_restrict = 0;
>> +   this->fields[this->num_fields].image_format = 0;
>>     this->fields[this->num_fields].explicit_xfb_buffer = 0;
>>     this->fields[this->num_fields].xfb_buffer = -1;
>>     this->fields[this->num_fields].xfb_stride = -1;
>> diff --git a/src/compiler/glsl_types.cpp b/src/compiler/glsl_types.cpp
>> index 00a95d4a8e..a495366919 100644
>> --- a/src/compiler/glsl_types.cpp
>> +++ b/src/compiler/glsl_types.cpp
>> @@ -955,6 +955,9 @@ glsl_type::record_compare(const glsl_type *b, bool
>> match_locations) const
>>        if (this->fields.structure[i].memory_restrict
>>            != b->fields.structure[i].memory_restrict)
>>           return false;
>> +      if (this->fields.structure[i].image_format
>> +          != b->fields.structure[i].image_format)
>> +         return false;
>>        if (this->fields.structure[i].precision
>>            != b->fields.structure[i].precision)
>>           return false;
>> diff --git a/src/compiler/glsl_types.h b/src/compiler/glsl_types.h
>> index 0449087238..67c152119f 100644
>> --- a/src/compiler/glsl_types.h
>> +++ b/src/compiler/glsl_types.h
>> @@ -978,6 +978,11 @@ struct glsl_struct_field {
>>     unsigned memory_restrict:1;
>>
>>     /**
>> +    * Layout format, applicable to image variables only.
>> +    */
>> +   unsigned image_format:16;
>> +
>> +   /**
>>      * Any of the xfb_* qualifiers trigger the shader to be in transform
>>      * feedback mode so we need to keep track of whether the buffer was
>>      * explicitly set or if its just been assigned the default global
>> value.
>> @@ -992,7 +997,8 @@ struct glsl_struct_field {
>>          sample(0), matrix_layout(GLSL_MATRIX_LAYOUT_INHERITED),
>> patch(0),
>>          precision(GLSL_PRECISION_NONE), memory_read_only(0),
>>          memory_write_only(0), memory_coherent(0), memory_volatile(0),
>> -        memory_restrict(0), explicit_xfb_buffer(0),
>> implicit_sized_array(0)
>> +        memory_restrict(0), image_format(0), explicit_xfb_buffer(0),
>> +        implicit_sized_array(0)
>>     {
>>        /* empty */
>>     }
>> --
>> 2.12.2
>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev


-- 
Lerne, wie die Welt wirklich ist,
Aber vergiss niemals, wie sie sein sollte.