[Mesa-dev] [RFC PATCH 08/26] glsl: select bindless sampler/image types on the fly

[Samuel Pitoiset]

On 04/12/2017 02:04 AM, Timothy Arceri wrote:
> 
> 
> On 12/04/17 02:48, Samuel Pitoiset wrote:
>> At this point, there is enough information to take the decision
>> to replace a sampler/image type by the corresponding bindless one.
>>
>> Signed-off-by: Samuel Pitoiset <samuel.pitoiset at gmail.com>
>> ---
>>  src/compiler/glsl/ast.h          | 10 ++++-
>>  src/compiler/glsl/ast_to_hir.cpp | 93 
>> ++++++++++++++++++++++++++++++++++++++--
>>  2 files changed, 98 insertions(+), 5 deletions(-)
>>
>> diff --git a/src/compiler/glsl/ast.h b/src/compiler/glsl/ast.h
>> index 9327e03979..2a2dd4bcd9 100644
>> --- a/src/compiler/glsl/ast.h
>> +++ b/src/compiler/glsl/ast.h
>> @@ -894,8 +894,14 @@ public:
>>     }
>>
>>     const struct glsl_type *glsl_type(const char **name,
>> -                     struct _mesa_glsl_parse_state *state)
>> -      const;
>> +                                     struct _mesa_glsl_parse_state 
>> *state,
>> +                                     bool is_interface_block = false) 
>> const;
>> +
>> +   bool is_temporary_storage() const;
>> +
>> +   bool is_bindless_type(const struct glsl_type *type,
>> +                         struct _mesa_glsl_parse_state *state,
>> +                         bool is_interface_block) const;
>>
>>     ast_type_qualifier qualifier;
>>     ast_type_specifier *specifier;
>> diff --git a/src/compiler/glsl/ast_to_hir.cpp 
>> b/src/compiler/glsl/ast_to_hir.cpp
>> index 5043d6a574..4a0f3fe315 100644
>> --- a/src/compiler/glsl/ast_to_hir.cpp
>> +++ b/src/compiler/glsl/ast_to_hir.cpp
>> @@ -2646,9 +2646,96 @@ select_gles_precision(unsigned qual_precision,
>>
>>  const glsl_type *
>>  ast_fully_specified_type::glsl_type(const char **name,
>> -                                    struct _mesa_glsl_parse_state 
>> *state) const
>> +                                    struct _mesa_glsl_parse_state 
>> *state,
>> +                                    bool is_interface_block) const
>>  {
>> -   return this->specifier->glsl_type(name, state);
>> +   const struct glsl_type *type;
>> +
>> +   type = this->specifier->glsl_type(name, state);
>> +   if (!type)
>> +      return NULL;
>> +
>> +   /* Bindless sampler/image types are replaced on-the-fly if needed as
>> +    * specified by ARB_bindless_texture. */
>> +   if (state->has_bindless()) {
> 
> Can we simplify this to:
> 
> if (state->has_bindless() &&
>      is_bindless_type(type, state, is_interface_block)) {
> 
> And drop the two calls below. By now we should have already thrown an 
> error if the bindless qualifiers were not applied to an image/sampler 
> right?

Yes, this can be simplied to:

if (state->has_bindless() &&
     is_bindless_type(type, state, is_interface_block)) {
   if (type->is_sampler())
     type = get_sampler_instance(...);
   if (type->is_image())
     type = get_image_instance(...);
}

But, we need to check if the type is sampler or image because of the 
global layout qualifier.

> 
>> +      if (type->is_sampler()) {
>> +         if (is_bindless_type(type, state, is_interface_block)) {
>> +            type = glsl_type::get_sampler_instance(
>> +                     (glsl_sampler_dim)type->sampler_dimensionality,
>> +                     type->sampler_shadow,
>> +                     type->sampler_array,
>> +                     (glsl_base_type)type->sampled_type,
>> +                     true);
>> +         }
>> +      }
>> +      if (type->is_image()) {
>> +         if (is_bindless_type(type, state, is_interface_block)) {
>> +            type = glsl_type::get_image_instance(
>> +                     (glsl_sampler_dim)type->sampler_dimensionality,
>> +                     type->sampler_array,
>> +                     (glsl_base_type)type->sampled_type,
>> +                     true);
>> +         }
>> +      }
>> +   }
>> +
>> +   return type;
>> +}
>> +
>> +bool
>> +ast_fully_specified_type::is_temporary_storage() const
>> +{
>> +   return (!qualifier.flags.q.in &&
>> +           !qualifier.flags.q.out &&
>> +           !qualifier.flags.q.attribute &&
>> +           !qualifier.flags.q.varying &&
>> +           !qualifier.flags.q.uniform &&
>> +           !qualifier.flags.q.buffer &&
>> +           !qualifier.flags.q.shared_storage);
>> +}

Just noticed, this should be moved to 
ast_type_qualifier::is_temporary_storage().

Will do in a separate patch just before this one.

>> +
>> +bool
>> +ast_fully_specified_type::is_bindless_type(const struct glsl_type *type,
>> +                                           struct 
>> _mesa_glsl_parse_state *state,
>> +                                           bool is_interface_block) 
>> const
>> +{
>> +   /* The bindless_sampler (and respectively bindless_image) layout 
>> qualifiers
>> +    * can be set as local scope, as well as at global scope. */
>> +   if (qualifier.flags.q.bindless_sampler ||
>> +       qualifier.flags.q.bindless_image ||
>> +       state->bindless_sampler_specified ||
>> +       state->bindless_image_specified)
>> +      return true;
>> +
>> +   /* The ARB_bindless_texture spec says:
>> +    *
>> +    * "Modify Section 4.3.7, Interface Blocks, p. 38"
>> +    *
>> +    * "(remove the following bullet from the last list on p. 39, thereby
>> +    *  permitting sampler types in interface blocks; image types are 
>> also
>> +    *  permitted in blocks by this extension)"
>> +    *
>> +    *    * sampler types are not allowed
>> +    */
>> +   if (is_interface_block)
>> +      return true;
>> +
>> +   /* The ARB_bindless_texture spec says:
>> +    *
>> +    * "Replace Section 4.1.7 (Samplers), p. 25"
>> +    *
>> +    * "Samplers may be declared as shader inputs and outputs, as uniform
>> +    *  variables, as temporary variables, and as function parameters"
>> +    *
>> +    * "Replace Section 4.1.X, (Images)"
>> +    *
>> +    * "Images may be declared as shader inputs and outputs, as uniform
>> +    *  variables, as temporary variables, and as function parameters."
>> +    */
>> +   if (qualifier.flags.q.in || qualifier.flags.q.out || 
>> is_temporary_storage())
>> +      return true;
>> +
>> +   return false;
>>  }
>>
>>  /**
>> @@ -6844,7 +6931,7 @@ 
>> ast_process_struct_or_iface_block_members(exec_list *instructions,
>>                            "embedded structure declarations are not 
>> allowed");
>>
>>        const glsl_type *decl_type =
>> -         decl_list->type->glsl_type(& type_name, state);
>> +         decl_list->type->glsl_type(& type_name, state, is_interface);
> 
> This looks like it should be a separate change?

Okay, I can put this into a separate patch before this one as well.

> 
>>
>>        const struct ast_type_qualifier *const qual =
>>           &decl_list->type->qualifier;
>>