On 26/06/17 20:28, Nicolai Hähnle wrote:

On 26.06.2017 12:18, Timothy Arceri wrote:
On 26/06/17 19:40, Nicolai Hähnle wrote:

From: Nicolai Hähnle <nicolai.haeh...@amd.com>

Save some passes over the IR.
---
src/compiler/glsl/linker.cpp | 88 +++++++++++++++++++++++++-------------------
  1 file changed, 50 insertions(+), 38 deletions(-)

diff --git a/src/compiler/glsl/linker.cpp b/src/compiler/glsl/linker.cpp
index 5366200..cfda263 100644
--- a/src/compiler/glsl/linker.cpp
+++ b/src/compiler/glsl/linker.cpp
@@ -88,75 +88,82 @@
  #include "main/enums.h"
  namespace {
  /**
   * Visitor that determines whether or not a variable is ever written.
   */
  class find_assignment_visitor : public ir_hierarchical_visitor {
  public:
-   find_assignment_visitor(const char *name)
-      : name(name), found(false)
+ find_assignment_visitor(unsigned int num_names, const char * const *names)
+      : num_names(num_names), names(names), found(0)
     {
-      /* empty */
+      assert(num_names <= 32);
     }
     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()
+   bool variable_found(unsigned idx)
     {
-      return found;
+      return found & (1u << idx);
     }
  private:
- const char *name; /**< Find writes to a variable with this name. */
-   bool found;             /**< Was a write to the variable found? */
+   ir_visitor_status check_variable_name(const char *name)
+   {
+      for (unsigned i = 0; i < num_names; ++i) {
+         if (strcmp(names[i], name) == 0) {
+            found |= 1u << i;
+            if (found == u_bit_consecutive(0, num_names))
+               return visit_stop;
+            break;
+         }
+      }
+
+      return visit_continue_with_parent;
+   }
+
+private:
+   unsigned int num_names;
+ const char * const * names; /**< Find writes to variables with these names. */ + uint32_t found; /**< Was a write to the variable found? */
  };
  /**
   * 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)
@@ -560,61 +567,63 @@ 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");
+      static const char * const variable_names[] = {
+         "gl_ClipDistance",
+         "gl_CullDistance",
+         "gl_ClipVertex"
+      };
+
+ find_assignment_visitor clipcull_visitor(!prog->IsES ? 3 : 2, variable_names);
-      clip_distance.run(shader->ir);
-      cull_distance.run(shader->ir);
+      clipcull_visitor.run(shader->ir);
        /* 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 (clipcull_visitor.variable_found(2) &&
+             clipcull_visitor.variable_found(0)) {

The 0, 2 etc is kind of confusing if you are browsing the code without knowledge of where the come from. Although I'm not sure what to suggest.

Yeah, I tend to agree and wasn't certain about this patch. I'll think about it some more, and depending on if I come up with something I might just drop it.

Probably overkill but you could use string_to_uint_map.



Maybe Emil or someone else can comment of patch 6. Everything else (same comment in patch 5) is:

Reviewed-by: Timothy Arceri <tarc...@itsqueeze.com>

Thanks!
Nicolai



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 (clipcull_visitor.variable_found(2) &&
+             clipcull_visitor.variable_found(1)) {
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 (clipcull_visitor.variable_found(0)) {
           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 (clipcull_visitor.variable_found(1)) {
           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 +678,24 @@ 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");
+      const char *gl_Position = "gl_Position";
+      find_assignment_visitor find(1, &gl_Position);
        find.run(shader->ir);
-      if (!find.variable_found()) {
+      if (!find.variable_found(0)) {
          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 +725,29 @@ 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");
+   static const char * const variable_names[] = {
+      "gl_FragColor",
+      "gl_FragData"
+   };
+   find_assignment_visitor frag_colordata(2, variable_names);
-   frag_color.run(shader->ir);
-   frag_data.run(shader->ir);
+   frag_colordata.run(shader->ir);
-   if (frag_color.variable_found() && frag_data.variable_found()) {
+ if (frag_colordata.variable_found(0) && frag_colordata.variable_found(1)) {
        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.




_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Reply via email to