glsl: fix shader_storage_blocks_write_access for SSBO block arrays (v2)

Submitted by Marek Olšák on April 16, 2019, 2:16 p.m.

Details

Message ID 20190416141624.11134-1-maraeo@gmail.com
State New
Headers show
Series "glsl: fix shader_storage_blocks_write_access for SSBO block arrays" ( rev: 2 ) in Mesa

Not browsing as part of any series.

Commit Message

Marek Olšák April 16, 2019, 2:16 p.m.
From: Marek Olšák <marek.olsak@amd.com>

This fixes KHR-GL45.compute_shader.resources-max on radeonsi.

Fixes: 4e1e8f684bf "glsl: remember which SSBOs are not read-only and pass it to gallium"

v2: use is_interface_array, protect again assertion failures in u_bit_consecutive
---
 src/compiler/glsl/link_uniforms.cpp | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/compiler/glsl/link_uniforms.cpp b/src/compiler/glsl/link_uniforms.cpp
index ef124111991..aa96227a7e1 100644
--- a/src/compiler/glsl/link_uniforms.cpp
+++ b/src/compiler/glsl/link_uniforms.cpp
@@ -515,44 +515,60 @@  public:
       this->record_next_bindless_sampler = new string_to_uint_map;
       this->record_next_image = new string_to_uint_map;
       this->record_next_bindless_image = new string_to_uint_map;
 
       buffer_block_index = -1;
       if (var->is_in_buffer_block()) {
          struct gl_uniform_block *blks = var->is_in_shader_storage_block() ?
             prog->data->ShaderStorageBlocks : prog->data->UniformBlocks;
          unsigned num_blks = var->is_in_shader_storage_block() ?
             prog->data->NumShaderStorageBlocks : prog->data->NumUniformBlocks;
+         bool is_interface_array =
+            var->is_interface_instance() && var->type->is_array();
 
-         if (var->is_interface_instance() && var->type->is_array()) {
+         if (is_interface_array) {
             unsigned l = strlen(var->get_interface_type()->name);
 
             for (unsigned i = 0; i < num_blks; i++) {
                if (strncmp(var->get_interface_type()->name, blks[i].Name, l)
                    == 0 && blks[i].Name[l] == '[') {
                   buffer_block_index = i;
                   break;
                }
             }
          } else {
             for (unsigned i = 0; i < num_blks; i++) {
                if (strcmp(var->get_interface_type()->name, blks[i].Name) == 0) {
                   buffer_block_index = i;
                   break;
                }
             }
          }
          assert(buffer_block_index != -1);
 
          if (var->is_in_shader_storage_block() &&
-             !var->data.memory_read_only)
-            shader_storage_blocks_write_access |= 1 << buffer_block_index;
+             !var->data.memory_read_only) {
+            unsigned array_size = is_interface_array ?
+                                     var->type->array_size() : 1;
+
+            STATIC_ASSERT(MAX_SHADER_STORAGE_BUFFERS <= 32);
+
+            /* Shaders that use too many SSBOs will fail to compile, which
+             * we don't care about.
+             *
+             * This is true for shaders that do not use too many SSBOs:
+             */
+            if (buffer_block_index + array_size <= 32) {
+               shader_storage_blocks_write_access |=
+                  u_bit_consecutive(buffer_block_index, array_size);
+            }
+         }
 
          /* Uniform blocks that were specified with an instance name must be
           * handled a little bit differently.  The name of the variable is the
           * name used to reference the uniform block instead of being the name
           * of a variable within the block.  Therefore, searching for the name
           * within the block will fail.
           */
          if (var->is_interface_instance()) {
             ubo_byte_offset = 0;
             process(var->get_interface_type(),

Comments


I ran this via the intel-ci and it has 0 regressions, I've also looked
at this before in v1.

Reviewed-by: Dave Airlie <airlied@redhat.com>

On Tue, 23 Apr 2019 at 06:08, Marek Olšák <maraeo@gmail.com> wrote:
>
> Ping. Thanks.
>
> On Tue, Apr 16, 2019 at 10:16 AM Marek Olšák <maraeo@gmail.com> wrote:
>>
>> From: Marek Olšák <marek.olsak@amd.com>
>>
>> This fixes KHR-GL45.compute_shader.resources-max on radeonsi.
>>
>> Fixes: 4e1e8f684bf "glsl: remember which SSBOs are not read-only and pass it to gallium"
>>
>> v2: use is_interface_array, protect again assertion failures in u_bit_consecutive
>> ---
>>  src/compiler/glsl/link_uniforms.cpp | 22 +++++++++++++++++++---
>>  1 file changed, 19 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/compiler/glsl/link_uniforms.cpp b/src/compiler/glsl/link_uniforms.cpp
>> index ef124111991..aa96227a7e1 100644
>> --- a/src/compiler/glsl/link_uniforms.cpp
>> +++ b/src/compiler/glsl/link_uniforms.cpp
>> @@ -515,44 +515,60 @@ public:
>>        this->record_next_bindless_sampler = new string_to_uint_map;
>>        this->record_next_image = new string_to_uint_map;
>>        this->record_next_bindless_image = new string_to_uint_map;
>>
>>        buffer_block_index = -1;
>>        if (var->is_in_buffer_block()) {
>>           struct gl_uniform_block *blks = var->is_in_shader_storage_block() ?
>>              prog->data->ShaderStorageBlocks : prog->data->UniformBlocks;
>>           unsigned num_blks = var->is_in_shader_storage_block() ?
>>              prog->data->NumShaderStorageBlocks : prog->data->NumUniformBlocks;
>> +         bool is_interface_array =
>> +            var->is_interface_instance() && var->type->is_array();
>>
>> -         if (var->is_interface_instance() && var->type->is_array()) {
>> +         if (is_interface_array) {
>>              unsigned l = strlen(var->get_interface_type()->name);
>>
>>              for (unsigned i = 0; i < num_blks; i++) {
>>                 if (strncmp(var->get_interface_type()->name, blks[i].Name, l)
>>                     == 0 && blks[i].Name[l] == '[') {
>>                    buffer_block_index = i;
>>                    break;
>>                 }
>>              }
>>           } else {
>>              for (unsigned i = 0; i < num_blks; i++) {
>>                 if (strcmp(var->get_interface_type()->name, blks[i].Name) == 0) {
>>                    buffer_block_index = i;
>>                    break;
>>                 }
>>              }
>>           }
>>           assert(buffer_block_index != -1);
>>
>>           if (var->is_in_shader_storage_block() &&
>> -             !var->data.memory_read_only)
>> -            shader_storage_blocks_write_access |= 1 << buffer_block_index;
>> +             !var->data.memory_read_only) {
>> +            unsigned array_size = is_interface_array ?
>> +                                     var->type->array_size() : 1;
>> +
>> +            STATIC_ASSERT(MAX_SHADER_STORAGE_BUFFERS <= 32);
>> +
>> +            /* Shaders that use too many SSBOs will fail to compile, which
>> +             * we don't care about.
>> +             *
>> +             * This is true for shaders that do not use too many SSBOs:
>> +             */
>> +            if (buffer_block_index + array_size <= 32) {
>> +               shader_storage_blocks_write_access |=
>> +                  u_bit_consecutive(buffer_block_index, array_size);
>> +            }
>> +         }
>>
>>           /* Uniform blocks that were specified with an instance name must be
>>            * handled a little bit differently.  The name of the variable is the
>>            * name used to reference the uniform block instead of being the name
>>            * of a variable within the block.  Therefore, searching for the name
>>            * within the block will fail.
>>            */
>>           if (var->is_interface_instance()) {
>>              ubo_byte_offset = 0;
>>              process(var->get_interface_type(),
>> --
>> 2.17.1
>>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev