[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