[Mesa-dev] [PATCH v2 04/31] glsl: make component_slots() returns 2 for samplers/images

[Samuel Pitoiset]

On 04/26/2017 09:10 AM, Nicolai Hähnle wrote:
> On 24.04.2017 12:35, Samuel Pitoiset wrote:
>> Bindless samplers/images are 64-bit unsigned integers, which
>> means they consume two components as specified by
>> ARB_bindless_texture.
>>
>> It looks like we are not wasting uniform storage by changing
>> this because default-block uniforms are not packed. So, if
>> we use N uint uniforms, they occupy N * 16 bytes in the
>> constant buffer. This is something that could be improved.
>>
>> Though, count_uniform_size needs to be adjusted to not count
>> a sampler (or image) twice.
>>
>> As a side effect, this will probably break the cache if you
>> have one because it will consider sampler/image types as
>> two components.
>>
>> Signed-off-by: Samuel Pitoiset <samuel.pitoiset at gmail.com>
>> ---
>>  src/compiler/glsl/link_uniforms.cpp | 8 ++++++--
>>  src/compiler/glsl_types.cpp         | 2 ++
>>  2 files changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/compiler/glsl/link_uniforms.cpp 
>> b/src/compiler/glsl/link_uniforms.cpp
>> index b462cb9d59..3331c85af4 100644
>> --- a/src/compiler/glsl/link_uniforms.cpp
>> +++ b/src/compiler/glsl/link_uniforms.cpp
>> @@ -340,9 +340,13 @@ private:
>>        if (type->contains_subroutine()) {
>>           this->num_shader_subroutines += values;
>>        } else if (type->contains_sampler()) {
>> -         this->num_shader_samplers += values;
>> +         /* Old-style (or bound) samplers are counted as two 
>> components as
>> +          * specified by ARB_bindless_texture. */
> 
> I think this comment is a bit confusing. All samplers are counted as two 
> components by these changes.

Right, I missed it at this point, but it's fixed later in the series.

> 
> Question: Are samplers allowed in structs? If yes, then this code will 
> need fixing to account for it. Either way, it would be good to add a 
> corresponding test to piglit (whether negative or positive).

They are allowed. I asked the spec authors directly.

> 
> I don't think the spec is clear on this. The general language on 
> structures (Section 4.1.8 of GLSL 4.5) doesn't restrict the types in 
> structs.
> 
> The introduction of section 4.1.7 of GLSL 4.5 (Opaque types) implicitly 
> says opaque types are not allowed in structs. The problem is that 
> ARB_bindless_texture is written against GLSL 4.0, and a general "Opaque 
> types" section simply doesn't exist there. There is a list in the 
> replacement "Samplers" section which is probably meant to be exhaustive, 
> which would suggest that samplers in structs are forbidden.

I do agree with you, the spec is unclear but it's allowed to declare 
samplers/images in structs.

> 
> Cheers,
> Nicolai
> 
> 
>> +         this->num_shader_samplers += values / 2;
>>        } else if (type->contains_image()) {
>> -         this->num_shader_images += values;
>> +         /* Old-style (or bound) images are counted as two components as
>> +          * specified by ARB_bindless_texture. */
>> +         this->num_shader_images += values / 2;
>>
>>           /* As drivers are likely to represent image uniforms as
>>            * scalar indices, count them against the limit of uniform
>> diff --git a/src/compiler/glsl_types.cpp b/src/compiler/glsl_types.cpp
>> index 0480bef80e..df148cfd21 100644
>> --- a/src/compiler/glsl_types.cpp
>> +++ b/src/compiler/glsl_types.cpp
>> @@ -1298,6 +1298,8 @@ glsl_type::component_slots() const
>>
>>     case GLSL_TYPE_SAMPLER:
>>     case GLSL_TYPE_IMAGE:
>> +      return 2;
>> +
>>     case GLSL_TYPE_SUBROUTINE:
>>        return 1;
>>
>>
> 
>