It should definitely be in 17.3.
On Thu, Oct 26, 2017 at 8:43 AM, Andres Gomez <ago...@igalia.com> wrote: > Ilia, for stable, this is quite a big patch, adding a new class and > source file. > > As per documentation, our criteria to accept a patch in the stable > queue includes not being larger than 100 lines, which this ones exceeds > by far: > https://www.mesa3d.org/submittingpatches.html#criteria > > Hence, by now I'm inclined to reject it. > > Let me know what you think. > > > On Sun, 2017-10-22 at 17:37 -0400, 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.chevi...@polymtl.ca> >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=103393 >> Cc: mesa-sta...@lists.freedesktop.org >> Signed-off-by: Ilia Mirkin <imir...@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; >> > -- > Br, > > Andres _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
