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

[Timothy Arceri]

On 12/04/17 19:49, Samuel Pitoiset wrote:
>
>
> 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.


Yep, that's what I meant. Thanks :)


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


Cool. I was going to ask about this but seem to have missed it.


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


Cool. I just wasn't sure if this was a general fix, or if it was 
specific to bindless somehow. A separate patch with a description would 
solve that. Thanks.


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