[Mesa-dev] [PATCH v2 1/4] glsl: look for multiple variables simultaneously with find_assignment_visitor
Timothy Arceri
tarceri at itsqueeze.com
Wed Jul 5 00:54:57 UTC 2017
On 04/07/17 19:37, Nicolai Hähnle wrote:
> On 03.07.2017 14:34, Nicolai Hähnle wrote:
>> From: Nicolai Hähnle <nicolai.haehnle at amd.com>
>>
>> Save some passes over the IR.
>>
>> v2: redesign to make the users of find_assignments more readable
>>
>> Reviewed-by: Ian Romanick <ian.d.romanick at intel.com> (v1)
>> ---
>> src/compiler/glsl/linker.cpp | 120
>> ++++++++++++++++++++++++++++---------------
>> 1 file changed, 79 insertions(+), 41 deletions(-)
>>
>> diff --git a/src/compiler/glsl/linker.cpp b/src/compiler/glsl/linker.cpp
>> index 15295da..4aa4682 100644
>> --- a/src/compiler/glsl/linker.cpp
>> +++ b/src/compiler/glsl/linker.cpp
>> @@ -83,82 +83,120 @@
>> #include "ir_uniform.h"
>> #include "builtin_functions.h"
>> #include "shader_cache.h"
>> #include "main/shaderobj.h"
>> #include "main/enums.h"
>> namespace {
>> +struct find_variable {
>> + const char *name;
>> + bool found;
>> +
>> + find_variable(const char *name) : name(name), found(false) {}
>> +};
>> +
>> /**
>> * Visitor that determines whether or not a variable is ever written.
>> + *
>> + * Use \ref find_assignments for convenience.
>> */
>> class find_assignment_visitor : public ir_hierarchical_visitor {
>> public:
>> - find_assignment_visitor(const char *name)
>> - : name(name), found(false)
>> + find_assignment_visitor(unsigned num_vars,
>> + find_variable * const *vars)
>> + : num_variables(num_vars), num_found(0), variables(vars)
>> {
>> - /* empty */
>> }
>> virtual ir_visitor_status visit_enter(ir_assignment *ir)
>> {
>> ir_variable *const var = ir->lhs->variable_referenced();
>> - if (strcmp(name, var->name) == 0) {
>> - found = true;
>> - return visit_stop;
>> - }
>> -
>> - return visit_continue_with_parent;
>> + return check_variable_name(var->name);
>> }
>> virtual ir_visitor_status visit_enter(ir_call *ir)
>> {
>> foreach_two_lists(formal_node, &ir->callee->parameters,
>> actual_node, &ir->actual_parameters) {
>> ir_rvalue *param_rval = (ir_rvalue *) actual_node;
>> ir_variable *sig_param = (ir_variable *) formal_node;
>> if (sig_param->data.mode == ir_var_function_out ||
>> sig_param->data.mode == ir_var_function_inout) {
>> ir_variable *var = param_rval->variable_referenced();
>> - if (var && strcmp(name, var->name) == 0) {
>> - found = true;
>> + if (var && check_variable_name(var->name) == visit_stop)
>> return visit_stop;
>> - }
>> }
>> }
>> if (ir->return_deref != NULL) {
>> ir_variable *const var =
>> ir->return_deref->variable_referenced();
>> - if (strcmp(name, var->name) == 0) {
>> - found = true;
>> + if (check_variable_name(var->name) == visit_stop)
>> return visit_stop;
>> - }
>> }
>> return visit_continue_with_parent;
>> }
>> - bool variable_found()
>> +private:
>> + ir_visitor_status check_variable_name(const char *name)
>> {
>> - return found;
>> + for (unsigned i = 0; i < num_variables; ++i) {
>> + if (strcmp(variables[i]->name, name) == 0) {
>> + if (variables[i]->found) {
>
> This should be !variables[i]->found. Fixed locally.
>
> Cheers,
> Nicolai
>
>
>> + variables[i]->found = true;
>> + if (++num_found >= num_variables)
This is a little confusing. We should never get:
++num_found > num_variables
I suspect this is left over from the fix above. Can we change this to:
assert(num_found + 1 <= num_variables);
if (++num_found == num_variables)
Up to you if you keep the assert() but it removes any doubt as to what
is going on here.
>> + return visit_stop;
>> + }
>> + break;
>> + }
>> + }
>> +
>> + return visit_continue_with_parent;
>> }
>> private:
>> - const char *name; /**< Find writes to a variable with this
>> name. */
>> - bool found; /**< Was a write to the variable found? */
>> + unsigned num_variables;
Can we add a small comment here /**< Number of variables to find. */
Or something similar, just to be 100% clear.
I agree this change is possibly overkill but it's much clearer now.
Thanks for reworking it. With these two small suggestions this is:
Reviewed-by: Timothy Arceri <tarceri at itsqueeze.com>
>> + unsigned num_found;
>> + find_variable * const *variables;
>> };
>> +/**
>> + * Determine whether or not any of NULL-terminated list of variables
>> is ever
>> + * written to.
>> + */
>> +static void
>> +find_assignments(exec_list *ir, find_variable * const *vars)
>> +{
>> + unsigned num_variables = 0;
>> +
>> + for (find_variable * const *v = vars; *v; ++v)
>> + num_variables++;
>> +
>> + find_assignment_visitor visitor(num_variables, vars);
>> + visitor.run(ir);
>> +}
>> +
>> +/**
>> + * Determine whether or not the given variable is ever written to.
>> + */
>> +static void
>> +find_assignments(exec_list *ir, find_variable *var)
>> +{
>> + find_assignment_visitor visitor(1, &var);
>> + visitor.run(ir);
>> +}
>> /**
>> * Visitor that determines whether or not a variable is ever read.
>> */
>> class find_deref_visitor : public ir_hierarchical_visitor {
>> public:
>> find_deref_visitor(const char *name)
>> : name(name), found(false)
>> {
>> /* empty */
>> @@ -560,61 +598,62 @@ analyze_clip_cull_usage(struct gl_shader_program
>> *prog,
>> /* From section 7.1 (Vertex Shader Special Variables) of the
>> * GLSL 1.30 spec:
>> *
>> * "It is an error for a shader to statically write both
>> * gl_ClipVertex and gl_ClipDistance."
>> *
>> * This does not apply to GLSL ES shaders, since GLSL ES
>> defines neither
>> * gl_ClipVertex nor gl_ClipDistance. However with
>> * GL_EXT_clip_cull_distance, this functionality is exposed in
>> ES 3.0.
>> */
>> - find_assignment_visitor clip_distance("gl_ClipDistance");
>> - find_assignment_visitor cull_distance("gl_CullDistance");
>> -
>> - clip_distance.run(shader->ir);
>> - cull_distance.run(shader->ir);
>> + find_variable gl_ClipDistance("gl_ClipDistance");
>> + find_variable gl_CullDistance("gl_CullDistance");
>> + find_variable gl_ClipVertex("gl_ClipVertex");
>> + find_variable * const variables[] = {
>> + &gl_ClipDistance,
>> + &gl_CullDistance,
>> + !prog->IsES ? &gl_ClipVertex : NULL,
>> + NULL
>> + };
>> + find_assignments(shader->ir, variables);
>> /* From the ARB_cull_distance spec:
>> *
>> * It is a compile-time or link-time error for the set of
>> shaders forming
>> * a program to statically read or write both gl_ClipVertex
>> and either
>> * gl_ClipDistance or gl_CullDistance.
>> *
>> * This does not apply to GLSL ES shaders, since GLSL ES
>> doesn't define
>> * gl_ClipVertex.
>> */
>> if (!prog->IsES) {
>> - find_assignment_visitor clip_vertex("gl_ClipVertex");
>> -
>> - clip_vertex.run(shader->ir);
>> -
>> - if (clip_vertex.variable_found() &&
>> clip_distance.variable_found()) {
>> + if (gl_ClipVertex.found && gl_ClipDistance.found) {
>> linker_error(prog, "%s shader writes to both
>> `gl_ClipVertex' "
>> "and `gl_ClipDistance'\n",
>> _mesa_shader_stage_to_string(shader->Stage));
>> return;
>> }
>> - if (clip_vertex.variable_found() &&
>> cull_distance.variable_found()) {
>> + if (gl_ClipVertex.found && gl_CullDistance.found) {
>> linker_error(prog, "%s shader writes to both
>> `gl_ClipVertex' "
>> "and `gl_CullDistance'\n",
>> _mesa_shader_stage_to_string(shader->Stage));
>> return;
>> }
>> }
>> - if (clip_distance.variable_found()) {
>> + if (gl_ClipDistance.found) {
>> ir_variable *clip_distance_var =
>> shader->symbols->get_variable("gl_ClipDistance");
>> assert(clip_distance_var);
>> *clip_distance_array_size = clip_distance_var->type->length;
>> }
>> - if (cull_distance.variable_found()) {
>> + if (gl_CullDistance.found) {
>> ir_variable *cull_distance_var =
>> shader->symbols->get_variable("gl_CullDistance");
>> assert(cull_distance_var);
>> *cull_distance_array_size = cull_distance_var->type->length;
>> }
>> /* From the ARB_cull_distance spec:
>> *
>> * It is a compile-time or link-time error for the set of
>> shaders forming
>> * a program to have the sum of the sizes of the
>> gl_ClipDistance and
>> * gl_CullDistance arrays to be larger than
>> @@ -669,23 +708,23 @@ validate_vertex_shader_executable(struct
>> gl_shader_program *prog,
>> * after being written. This value will be used by primitive
>> * assembly, clipping, culling, and other fixed functionality
>> * operations, if present, that operate on primitives after
>> * vertex processing has occurred. Its value is undefined if
>> * the vertex shader executable does not write gl_Position."
>> *
>> * All GLSL ES Versions are similar to GLSL 1.40--failing to
>> write to
>> * gl_Position is not an error.
>> */
>> if (prog->data->Version < (prog->IsES ? 300 : 140)) {
>> - find_assignment_visitor find("gl_Position");
>> - find.run(shader->ir);
>> - if (!find.variable_found()) {
>> + find_variable gl_Position("gl_Position");
>> + find_assignments(shader->ir, &gl_Position);
>> + if (!gl_Position.found) {
>> if (prog->IsES) {
>> linker_warning(prog,
>> "vertex shader does not write to
>> `gl_Position'. "
>> "Its value is undefined. \n");
>> } else {
>> linker_error(prog,
>> "vertex shader does not write to
>> `gl_Position'. \n");
>> }
>> return;
>> }
>> @@ -715,27 +754,26 @@ validate_tess_eval_shader_executable(struct
>> gl_shader_program *prog,
>> *
>> * \param shader Fragment shader executable to be verified
>> */
>> void
>> validate_fragment_shader_executable(struct gl_shader_program *prog,
>> struct gl_linked_shader *shader)
>> {
>> if (shader == NULL)
>> return;
>> - find_assignment_visitor frag_color("gl_FragColor");
>> - find_assignment_visitor frag_data("gl_FragData");
>> -
>> - frag_color.run(shader->ir);
>> - frag_data.run(shader->ir);
>> + find_variable gl_FragColor("gl_FragColor");
>> + find_variable gl_FragData("gl_FragData");
>> + find_variable * const variables[] = { &gl_FragColor, &gl_FragData,
>> NULL };
>> + find_assignments(shader->ir, variables);
>> - if (frag_color.variable_found() && frag_data.variable_found()) {
>> + if (gl_FragColor.found && gl_FragData.found) {
>> linker_error(prog, "fragment shader writes to both "
>> "`gl_FragColor' and `gl_FragData'\n");
>> }
>> }
>> /**
>> * Verify that a geometry shader executable meets all semantic
>> requirements
>> *
>> * Also sets prog->Geom.VerticesIn, and
>> info.clip_distance_array_sizeand
>> * info.cull_distance_array_size as a side effect.
>>
>
>
More information about the mesa-dev
mailing list