[Mesa-dev] [PATCH 1/2] glsl: store the image format in glsl_struct_field
Jason Ekstrand
jason at jlekstrand.net
Mon May 8 18:01:26 UTC 2017
On Mon, May 8, 2017 at 3:26 AM, Nicolai Hähnle <nhaehnle at gmail.com> wrote:
> 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.
>>
>
Yes, intrinsic signatures would be a pain. I think I'm won over by that
argument. We could do some sort of "interface type" thing where we have
one type that's used for actually decoding the image and another type
that's used for function signature matching. However, that seems a bit
insane.
> 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
>
Which spec? :-) The GLSL types system is shared by two different
front-ends and SPIR-V definitely makes format part of the type.
> 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.
>
Yeah, that's ugly...
As I said above, I think I agree that this is the best we can do for now.
Hopefully, we'll have the opportunity to refactor some things and fix that
in the future.
> 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.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170508/bce378ea/attachment.html>
More information about the mesa-dev
mailing list