[Mesa-stable] [Mesa-dev] [PATCH v2] glsl: fix derived cs variables

Samuel Pitoiset samuel.pitoiset at gmail.com
Mon Oct 23 10:12:25 UTC 2017


Reviewed-by: Samuel Pitoiset <samuel.pitoiset at gmail.com>

On 10/22/2017 11:37 PM, Ilia Mirkin wrote:
> There are two issues with the current implementation. First, it relies
> on the layout(local_size_*) happening in the same shader as the main
> function, and secondly it doesn't work for variable group sizes.
> 
> In both cases, the simplest fix is to move the setup of these derived
> values to a later time, similar to how the gl_VertexID workarounds are
> done. There already exist system values defined for both of the derived
> values, so we use them unconditionally, and lower them after linking is
> performed.
> 
> While we're at it, we move to using gl_LocalGroupSizeARB instead of
> gl_WorkGroupSize for variable group sizes.
> 
> Also the dead code elimination avoidance can be removed, since there
> can be situations where gl_LocalGroupSizeARB is needed but has not been
> inserted for the shader with main function. As a result, the lowering
> code has to insert its own copies of the system values if needed.
> 
> Reported-by: Stephane Chevigny <stephane.chevigny at polymtl.ca>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=103393
> Cc: mesa-stable at lists.freedesktop.org
> Signed-off-by: Ilia Mirkin <imirkin at alum.mit.edu>
> ---
> 
> Well this all turned out to be a deeper rabbit-hole than I was anticipating.
> I'm going to write up some piglit tests for these cases as they are fairly
> subtle, and unlikely to occur in practice.
> 
> I figured it was easier to just re-add the system values instead of having
> the dead code not-remove them, since gl_LocalGroupSizeARB might not be in
> the shader with the main function (it's only added if the variable size ext
> is enabled, which it might only be in a second shader which sets the
> local_size_*). So we'd have to have it anyways, might as well do it for all.
> 
>   src/compiler/Makefile.sources                    |   1 +
>   src/compiler/glsl/builtin_variables.cpp          |  94 +--------
>   src/compiler/glsl/glsl_parser_extras.cpp         |   2 -
>   src/compiler/glsl/ir.h                           |   4 -
>   src/compiler/glsl/ir_optimization.h              |   1 +
>   src/compiler/glsl/linker.cpp                     |   3 +
>   src/compiler/glsl/lower_cs_derived.cpp           | 234 +++++++++++++++++++++++
>   src/compiler/glsl/meson.build                    |   1 +
>   src/compiler/glsl/opt_dead_builtin_variables.cpp |  22 ---
>   9 files changed, 244 insertions(+), 118 deletions(-)
>   create mode 100644 src/compiler/glsl/lower_cs_derived.cpp
> 
> diff --git a/src/compiler/Makefile.sources b/src/compiler/Makefile.sources
> index 2724a41286e..27cc33ab835 100644
> --- a/src/compiler/Makefile.sources
> +++ b/src/compiler/Makefile.sources
> @@ -85,6 +85,7 @@ LIBGLSL_FILES = \
>   	glsl/lower_buffer_access.cpp \
>   	glsl/lower_buffer_access.h \
>   	glsl/lower_const_arrays_to_uniforms.cpp \
> +	glsl/lower_cs_derived.cpp \
>   	glsl/lower_discard.cpp \
>   	glsl/lower_discard_flow.cpp \
>   	glsl/lower_distance.cpp \
> diff --git a/src/compiler/glsl/builtin_variables.cpp b/src/compiler/glsl/builtin_variables.cpp
> index ea2d897cc8e..00bc99dd619 100644
> --- a/src/compiler/glsl/builtin_variables.cpp
> +++ b/src/compiler/glsl/builtin_variables.cpp
> @@ -1295,15 +1295,10 @@ builtin_variable_generator::generate_cs_special_vars()
>                          uvec3_t, "gl_LocalGroupSizeARB");
>      }
>   
> -   if (state->ctx->Const.LowerCsDerivedVariables) {
> -      add_variable("gl_GlobalInvocationID", uvec3_t, ir_var_auto, 0);
> -      add_variable("gl_LocalInvocationIndex", uint_t, ir_var_auto, 0);
> -   } else {
> -      add_system_value(SYSTEM_VALUE_GLOBAL_INVOCATION_ID,
> -                       uvec3_t, "gl_GlobalInvocationID");
> -      add_system_value(SYSTEM_VALUE_LOCAL_INVOCATION_INDEX,
> -                       uint_t, "gl_LocalInvocationIndex");
> -   }
> +   add_system_value(SYSTEM_VALUE_GLOBAL_INVOCATION_ID,
> +                    uvec3_t, "gl_GlobalInvocationID");
> +   add_system_value(SYSTEM_VALUE_LOCAL_INVOCATION_INDEX,
> +                    uint_t, "gl_LocalInvocationIndex");
>   }
>   
>   
> @@ -1474,84 +1469,3 @@ _mesa_glsl_initialize_variables(exec_list *instructions,
>         break;
>      }
>   }
> -
> -
> -/**
> - * Initialize compute shader variables with values that are derived from other
> - * compute shader variable.
> - */
> -static void
> -initialize_cs_derived_variables(gl_shader *shader,
> -                                ir_function_signature *const main_sig)
> -{
> -   assert(shader->Stage == MESA_SHADER_COMPUTE);
> -
> -   ir_variable *gl_GlobalInvocationID =
> -      shader->symbols->get_variable("gl_GlobalInvocationID");
> -   assert(gl_GlobalInvocationID);
> -   ir_variable *gl_WorkGroupID =
> -      shader->symbols->get_variable("gl_WorkGroupID");
> -   assert(gl_WorkGroupID);
> -   ir_variable *gl_WorkGroupSize =
> -      shader->symbols->get_variable("gl_WorkGroupSize");
> -   if (gl_WorkGroupSize == NULL) {
> -      void *const mem_ctx = ralloc_parent(shader->ir);
> -      gl_WorkGroupSize = new(mem_ctx) ir_variable(glsl_type::uvec3_type,
> -                                                  "gl_WorkGroupSize",
> -                                                  ir_var_auto);
> -      gl_WorkGroupSize->data.how_declared = ir_var_declared_implicitly;
> -      gl_WorkGroupSize->data.read_only = true;
> -      shader->ir->push_head(gl_WorkGroupSize);
> -   }
> -   ir_variable *gl_LocalInvocationID =
> -      shader->symbols->get_variable("gl_LocalInvocationID");
> -   assert(gl_LocalInvocationID);
> -
> -   /* gl_GlobalInvocationID =
> -    *    gl_WorkGroupID * gl_WorkGroupSize + gl_LocalInvocationID
> -    */
> -   ir_instruction *inst =
> -      assign(gl_GlobalInvocationID,
> -             add(mul(gl_WorkGroupID, gl_WorkGroupSize),
> -                 gl_LocalInvocationID));
> -   main_sig->body.push_head(inst);
> -
> -   /* gl_LocalInvocationIndex =
> -    *    gl_LocalInvocationID.z * gl_WorkGroupSize.x * gl_WorkGroupSize.y +
> -    *    gl_LocalInvocationID.y * gl_WorkGroupSize.x +
> -    *    gl_LocalInvocationID.x;
> -    */
> -   ir_expression *index_z =
> -      mul(mul(swizzle_z(gl_LocalInvocationID), swizzle_x(gl_WorkGroupSize)),
> -          swizzle_y(gl_WorkGroupSize));
> -   ir_expression *index_y =
> -      mul(swizzle_y(gl_LocalInvocationID), swizzle_x(gl_WorkGroupSize));
> -   ir_expression *index_y_plus_z = add(index_y, index_z);
> -   operand index_x(swizzle_x(gl_LocalInvocationID));
> -   ir_expression *index_x_plus_y_plus_z = add(index_y_plus_z, index_x);
> -   ir_variable *gl_LocalInvocationIndex =
> -      shader->symbols->get_variable("gl_LocalInvocationIndex");
> -   assert(gl_LocalInvocationIndex);
> -   inst = assign(gl_LocalInvocationIndex, index_x_plus_y_plus_z);
> -   main_sig->body.push_head(inst);
> -}
> -
> -
> -/**
> - * Initialize builtin variables with values based on other builtin variables.
> - * These are initialized in the main function.
> - */
> -void
> -_mesa_glsl_initialize_derived_variables(struct gl_context *ctx,
> -                                        gl_shader *shader)
> -{
> -   /* We only need to set CS variables currently. */
> -   if (shader->Stage == MESA_SHADER_COMPUTE &&
> -       ctx->Const.LowerCsDerivedVariables) {
> -      ir_function_signature *const main_sig =
> -         _mesa_get_main_function_signature(shader->symbols);
> -
> -      if (main_sig != NULL)
> -         initialize_cs_derived_variables(shader, main_sig);
> -   }
> -}
> diff --git a/src/compiler/glsl/glsl_parser_extras.cpp b/src/compiler/glsl/glsl_parser_extras.cpp
> index 764c05ad802..51d835bc535 100644
> --- a/src/compiler/glsl/glsl_parser_extras.cpp
> +++ b/src/compiler/glsl/glsl_parser_extras.cpp
> @@ -2009,8 +2009,6 @@ opt_shader_and_create_symbol_table(struct gl_context *ctx,
>            break;
>         }
>      }
> -
> -   _mesa_glsl_initialize_derived_variables(ctx, shader);
>   }
>   
>   void
> diff --git a/src/compiler/glsl/ir.h b/src/compiler/glsl/ir.h
> index 27eafe8e22b..070d7b3ffdd 100644
> --- a/src/compiler/glsl/ir.h
> +++ b/src/compiler/glsl/ir.h
> @@ -2413,10 +2413,6 @@ _mesa_glsl_initialize_variables(exec_list *instructions,
>   				struct _mesa_glsl_parse_state *state);
>   
>   extern void
> -_mesa_glsl_initialize_derived_variables(struct gl_context *ctx,
> -                                        gl_shader *shader);
> -
> -extern void
>   reparent_ir(exec_list *list, void *mem_ctx);
>   
>   extern void
> diff --git a/src/compiler/glsl/ir_optimization.h b/src/compiler/glsl/ir_optimization.h
> index eb3ec3b0c7d..f44ddcb05be 100644
> --- a/src/compiler/glsl/ir_optimization.h
> +++ b/src/compiler/glsl/ir_optimization.h
> @@ -166,6 +166,7 @@ void optimize_dead_builtin_variables(exec_list *instructions,
>   bool lower_tess_level(gl_linked_shader *shader);
>   
>   bool lower_vertex_id(gl_linked_shader *shader);
> +bool lower_cs_derived(gl_linked_shader *shader);
>   bool lower_blend_equation_advanced(gl_linked_shader *shader);
>   
>   bool lower_subroutine(exec_list *instructions, struct _mesa_glsl_parse_state *state);
> diff --git a/src/compiler/glsl/linker.cpp b/src/compiler/glsl/linker.cpp
> index 03eb05bf637..f72bff53bf5 100644
> --- a/src/compiler/glsl/linker.cpp
> +++ b/src/compiler/glsl/linker.cpp
> @@ -2374,6 +2374,9 @@ link_intrastage_shaders(void *mem_ctx,
>      if (ctx->Const.VertexID_is_zero_based)
>         lower_vertex_id(linked);
>   
> +   if (ctx->Const.LowerCsDerivedVariables)
> +      lower_cs_derived(linked);
> +
>   #ifdef DEBUG
>      /* Compute the source checksum. */
>      linked->SourceChecksum = 0;
> diff --git a/src/compiler/glsl/lower_cs_derived.cpp b/src/compiler/glsl/lower_cs_derived.cpp
> new file mode 100644
> index 00000000000..f7ec2a48b47
> --- /dev/null
> +++ b/src/compiler/glsl/lower_cs_derived.cpp
> @@ -0,0 +1,234 @@
> +/*
> + * Copyright © 2017 Ilia Mirkin
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> + * DEALINGS IN THE SOFTWARE.
> + */
> +
> +/**
> + * \file lower_cs_derived.cpp
> + *
> + * For hardware that does not support the gl_GlobalInvocationID and
> + * gl_LocalInvocationIndex system values, replace them with fresh
> + * globals. Note that we can't rely on gl_WorkGroupSize or
> + * gl_LocalGroupSizeARB being available, since they may only have been defined
> + * in a non-main shader.
> + *
> + * [ This can happen if only a secondary shader has the layout(local_size_*)
> + *   declaration. ]
> + *
> + * This is meant to be run post-linking.
> + */
> +
> +#include "glsl_symbol_table.h"
> +#include "ir_hierarchical_visitor.h"
> +#include "ir.h"
> +#include "ir_builder.h"
> +#include "linker.h"
> +#include "program/prog_statevars.h"
> +#include "builtin_functions.h"
> +
> +using namespace ir_builder;
> +
> +namespace {
> +
> +class lower_cs_derived_visitor : public ir_hierarchical_visitor {
> +public:
> +   explicit lower_cs_derived_visitor(gl_linked_shader *shader)
> +      : progress(false),
> +        shader(shader),
> +        local_size_variable(shader->Program->info.cs.local_size_variable),
> +        gl_WorkGroupSize(NULL),
> +        gl_WorkGroupID(NULL),
> +        gl_LocalInvocationID(NULL),
> +        gl_GlobalInvocationID(NULL),
> +        gl_LocalInvocationIndex(NULL)
> +   {
> +      main_sig = _mesa_get_main_function_signature(shader->symbols);
> +      assert(main_sig);
> +   }
> +
> +   virtual ir_visitor_status visit(ir_dereference_variable *);
> +
> +   ir_variable *add_system_value(
> +         int slot, const glsl_type *type, const char *name);
> +   void find_sysvals();
> +   void make_gl_GlobalInvocationID();
> +   void make_gl_LocalInvocationIndex();
> +
> +   bool progress;
> +
> +private:
> +   gl_linked_shader *shader;
> +   bool local_size_variable;
> +   ir_function_signature *main_sig;
> +
> +   ir_rvalue *gl_WorkGroupSize;
> +   ir_variable *gl_WorkGroupID;
> +   ir_variable *gl_LocalInvocationID;
> +
> +   ir_variable *gl_GlobalInvocationID;
> +   ir_variable *gl_LocalInvocationIndex;
> +};
> +
> +} /* anonymous namespace */
> +
> +ir_variable *
> +lower_cs_derived_visitor::add_system_value(
> +      int slot, const glsl_type *type, const char *name)
> +{
> +   ir_variable *var = new(shader) ir_variable(type, name, ir_var_system_value);
> +   var->data.how_declared = ir_var_declared_implicitly;
> +   var->data.read_only = true;
> +   var->data.location = slot;
> +   var->data.explicit_location = true;
> +   var->data.explicit_index = 0;
> +   shader->ir->push_head(var);
> +
> +   return var;
> +}
> +
> +void
> +lower_cs_derived_visitor::find_sysvals()
> +{
> +   if (gl_WorkGroupSize != NULL)
> +      return;
> +
> +   ir_variable *WorkGroupSize;
> +   if (local_size_variable)
> +      WorkGroupSize = shader->symbols->get_variable("gl_LocalGroupSizeARB");
> +   else
> +      WorkGroupSize = shader->symbols->get_variable("gl_WorkGroupSize");
> +   if (WorkGroupSize)
> +      gl_WorkGroupSize = new(shader) ir_dereference_variable(WorkGroupSize);
> +   gl_WorkGroupID = shader->symbols->get_variable("gl_WorkGroupID");
> +   gl_LocalInvocationID = shader->symbols->get_variable("gl_LocalInvocationID");
> +
> +   /*
> +    * These may be missing due to either dead code elimination, or, in the
> +    * case of the group size, due to the layout being declared in a non-main
> +    * shader. Re-create them.
> +    */
> +
> +   if (!gl_WorkGroupID)
> +      gl_WorkGroupID = add_system_value(
> +            SYSTEM_VALUE_WORK_GROUP_ID, glsl_type::uvec3_type, "gl_WorkGroupID");
> +   if (!gl_LocalInvocationID)
> +      gl_LocalInvocationID = add_system_value(
> +            SYSTEM_VALUE_LOCAL_INVOCATION_ID, glsl_type::uvec3_type,
> +            "gl_LocalInvocationID");
> +   if (!WorkGroupSize) {
> +      if (local_size_variable) {
> +         gl_WorkGroupSize = new(shader) ir_dereference_variable(
> +               add_system_value(
> +                     SYSTEM_VALUE_LOCAL_GROUP_SIZE, glsl_type::uvec3_type,
> +                     "gl_LocalGroupSizeARB"));
> +      } else {
> +         ir_constant_data data;
> +         memset(&data, 0, sizeof(data));
> +         for (int i = 0; i < 3; i++)
> +            data.u[i] = shader->Program->info.cs.local_size[i];
> +         gl_WorkGroupSize = new(shader) ir_constant(glsl_type::uvec3_type, &data);
> +      }
> +   }
> +}
> +
> +void
> +lower_cs_derived_visitor::make_gl_GlobalInvocationID()
> +{
> +   if (gl_GlobalInvocationID != NULL)
> +      return;
> +
> +   find_sysvals();
> +
> +   /* gl_GlobalInvocationID =
> +    *    gl_WorkGroupID * gl_WorkGroupSize + gl_LocalInvocationID
> +    */
> +   gl_GlobalInvocationID = new(shader) ir_variable(
> +         glsl_type::uvec3_type, "__GlobalInvocationID", ir_var_temporary);
> +   shader->ir->push_head(gl_GlobalInvocationID);
> +
> +   ir_instruction *inst =
> +      assign(gl_GlobalInvocationID,
> +             add(mul(gl_WorkGroupID, gl_WorkGroupSize->clone(shader, NULL)),
> +                 gl_LocalInvocationID));
> +   main_sig->body.push_head(inst);
> +}
> +
> +void
> +lower_cs_derived_visitor::make_gl_LocalInvocationIndex()
> +{
> +   if (gl_LocalInvocationIndex != NULL)
> +      return;
> +
> +   find_sysvals();
> +
> +   /* gl_LocalInvocationIndex =
> +    *    gl_LocalInvocationID.z * gl_WorkGroupSize.x * gl_WorkGroupSize.y +
> +    *    gl_LocalInvocationID.y * gl_WorkGroupSize.x +
> +    *    gl_LocalInvocationID.x;
> +    */
> +   gl_LocalInvocationIndex = new(shader)
> +      ir_variable(glsl_type::uint_type, "__LocalInvocationIndex", ir_var_temporary);
> +   shader->ir->push_head(gl_LocalInvocationIndex);
> +
> +   ir_expression *index_z =
> +      mul(mul(swizzle_z(gl_LocalInvocationID), swizzle_x(gl_WorkGroupSize->clone(shader, NULL))),
> +          swizzle_y(gl_WorkGroupSize->clone(shader, NULL)));
> +   ir_expression *index_y =
> +      mul(swizzle_y(gl_LocalInvocationID), swizzle_x(gl_WorkGroupSize->clone(shader, NULL)));
> +   ir_expression *index_y_plus_z = add(index_y, index_z);
> +   operand index_x(swizzle_x(gl_LocalInvocationID));
> +   ir_expression *index_x_plus_y_plus_z = add(index_y_plus_z, index_x);
> +   ir_instruction *inst =
> +      assign(gl_LocalInvocationIndex, index_x_plus_y_plus_z);
> +   main_sig->body.push_head(inst);
> +}
> +
> +ir_visitor_status
> +lower_cs_derived_visitor::visit(ir_dereference_variable *ir)
> +{
> +   if (ir->var->data.mode == ir_var_system_value &&
> +       ir->var->data.location == SYSTEM_VALUE_GLOBAL_INVOCATION_ID) {
> +      make_gl_GlobalInvocationID();
> +      ir->var = gl_GlobalInvocationID;
> +      progress = true;
> +   }
> +
> +   if (ir->var->data.mode == ir_var_system_value &&
> +       ir->var->data.location == SYSTEM_VALUE_LOCAL_INVOCATION_INDEX) {
> +      make_gl_LocalInvocationIndex();
> +      ir->var = gl_LocalInvocationIndex;
> +      progress = true;
> +   }
> +
> +   return visit_continue;
> +}
> +
> +bool
> +lower_cs_derived(gl_linked_shader *shader)
> +{
> +   if (shader->Stage != MESA_SHADER_COMPUTE)
> +      return false;
> +
> +   lower_cs_derived_visitor v(shader);
> +   v.run(shader->ir);
> +
> +   return v.progress;
> +}
> diff --git a/src/compiler/glsl/meson.build b/src/compiler/glsl/meson.build
> index d1a75eb8c36..76fcafb9910 100644
> --- a/src/compiler/glsl/meson.build
> +++ b/src/compiler/glsl/meson.build
> @@ -124,6 +124,7 @@ files_libglsl = files(
>     'lower_buffer_access.cpp',
>     'lower_buffer_access.h',
>     'lower_const_arrays_to_uniforms.cpp',
> +  'lower_cs_derived.cpp',
>     'lower_discard.cpp',
>     'lower_discard_flow.cpp',
>     'lower_distance.cpp',
> diff --git a/src/compiler/glsl/opt_dead_builtin_variables.cpp b/src/compiler/glsl/opt_dead_builtin_variables.cpp
> index 03e578982b9..0d4e3a8f00a 100644
> --- a/src/compiler/glsl/opt_dead_builtin_variables.cpp
> +++ b/src/compiler/glsl/opt_dead_builtin_variables.cpp
> @@ -62,23 +62,6 @@ optimize_dead_builtin_variables(exec_list *instructions,
>          * information, so removing these variables from the user shader will
>          * cause problems later.
>          *
> -       * For compute shaders, gl_GlobalInvocationID has some dependencies, so
> -       * we avoid removing these dependencies.
> -       *
> -       * We also avoid removing gl_GlobalInvocationID at this stage because it
> -       * might be used by a linked shader. In this case it still needs to be
> -       * initialized by the main function.
> -       *
> -       *    gl_GlobalInvocationID =
> -       *       gl_WorkGroupID * gl_WorkGroupSize + gl_LocalInvocationID
> -       *
> -       * Similarly, we initialize gl_LocalInvocationIndex in the main function:
> -       *
> -       *    gl_LocalInvocationIndex =
> -       *       gl_LocalInvocationID.z * gl_WorkGroupSize.x * gl_WorkGroupSize.y +
> -       *       gl_LocalInvocationID.y * gl_WorkGroupSize.x +
> -       *       gl_LocalInvocationID.x;
> -       *
>          * Matrix uniforms with "Transpose" are not eliminated because there's
>          * an optimization pass that can turn references to the regular matrix
>          * into references to the transpose matrix.  Eliminating the transpose
> @@ -90,11 +73,6 @@ optimize_dead_builtin_variables(exec_list *instructions,
>          */
>         if (strcmp(var->name, "gl_ModelViewProjectionMatrix") == 0
>             || strcmp(var->name, "gl_Vertex") == 0
> -          || strcmp(var->name, "gl_WorkGroupID") == 0
> -          || strcmp(var->name, "gl_WorkGroupSize") == 0
> -          || strcmp(var->name, "gl_LocalInvocationID") == 0
> -          || strcmp(var->name, "gl_GlobalInvocationID") == 0
> -          || strcmp(var->name, "gl_LocalInvocationIndex") == 0
>             || strstr(var->name, "Transpose") != NULL)
>            continue;
>   
> 


More information about the mesa-stable mailing list