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

[Nicolai Hähnle]

On 08.05.2017 12:11, Nicolai Hähnle wrote:
> 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.

Another "on the other hand" is that it's a good idea for the compiler to 
mirror the spec, and the spec doesn't think of memory and format 
qualifiers as part of the type. E.g. you can say

   foo = texture(sampler2D(handle), ...);

but there isn't even grammar to say something like

   foo = imageLoad(layout(format=r32f) image2D(handle), ...);

so a round-trip via variables is necessary.

This is arguably a corner where the spec is just horrible, but it is 
what it is.

Cheers,
Nicolai


>
> 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.