From: Kenneth Graunke <kenneth@whitecape.org>
Date: Wed, 21 Sep 2016 09:03:10 +0000 (-0700)
Subject: glsl: Check TCS barrier restrictions at ast_to_hir time, not link time.
X-Git-Url: https://git.libre-soc.org/?a=commitdiff_plain;h=1617f59bc675d6e44d16ab56d95e910d2e2c8f4e;p=mesa.git

glsl: Check TCS barrier restrictions at ast_to_hir time, not link time.

We want to check prior to optimization - otherwise we might fail to
detect cases where barrier() is in control flow which is always taken
(and therefore gets optimized away).

We don't currently loop unroll if there are function calls inside;
otherwise we might have a problem detecting barrier() in loops that
get unrolled as well.

Tapani's switch handling code adds a loop around switch statements, so
even with the mess of if ladders, we'll properly reject it.

Enforcing these rules at compile time makes more sense more sense than
link time.  Doing it at ast-to-hir time (rather than as an IR pass)
allows us to emit an error message with proper line numbers.
(Otherwise, I would have preferred the IR pass...)

Fixes spec/arb_tessellation_shader/compiler/barrier-switch-always.tesc.

Signed-off-by: Kenneth Graunke <kenneth@whitecape.org>
Reviewed-by; Ian Romanick <ian.d.romanick@intel.com>
---

diff --git a/src/compiler/glsl/ast_function.cpp b/src/compiler/glsl/ast_function.cpp
index ea3ac87d810..7e62ab7d56d 100644
--- a/src/compiler/glsl/ast_function.cpp
+++ b/src/compiler/glsl/ast_function.cpp
@@ -2143,6 +2143,25 @@ ast_function_expression::hir(exec_list *instructions,
          /* an error has already been emitted */
          value = ir_rvalue::error_value(ctx);
       } else {
+         if (state->stage == MESA_SHADER_TESS_CTRL &&
+             sig->is_builtin() && strcmp(func_name, "barrier") == 0) {
+            if (state->current_function == NULL ||
+                strcmp(state->current_function->function_name(), "main") != 0) {
+               _mesa_glsl_error(&loc, state,
+                                "barrier() may only be used in main()");
+            }
+
+            if (state->found_return) {
+               _mesa_glsl_error(&loc, state,
+                                "barrier() may not be used after return");
+            }
+
+            if (instructions != &state->current_function->body) {
+               _mesa_glsl_error(&loc, state,
+                                "barrier() may not be used in control flow");
+            }
+         }
+
          value = generate_call(instructions, sig,
                                &actual_parameters, sub_var, array_idx, state);
          if (!value) {
diff --git a/src/compiler/glsl/linker.cpp b/src/compiler/glsl/linker.cpp
index ac4191f2cd5..02a8d0c86e4 100644
--- a/src/compiler/glsl/linker.cpp
+++ b/src/compiler/glsl/linker.cpp
@@ -260,97 +260,6 @@ public:
    }
 };
 
-class barrier_use_visitor : public ir_hierarchical_visitor {
-public:
-   barrier_use_visitor(gl_shader_program *prog)
-      : prog(prog), in_main(false), after_return(false), control_flow(0)
-   {
-   }
-
-   virtual ~barrier_use_visitor()
-   {
-      /* empty */
-   }
-
-   virtual ir_visitor_status visit_enter(ir_function *ir)
-   {
-      if (strcmp(ir->name, "main") == 0)
-         in_main = true;
-
-      return visit_continue;
-   }
-
-   virtual ir_visitor_status visit_leave(ir_function *)
-   {
-      in_main = false;
-      after_return = false;
-      return visit_continue;
-   }
-
-   virtual ir_visitor_status visit_leave(ir_return *)
-   {
-      after_return = true;
-      return visit_continue;
-   }
-
-   virtual ir_visitor_status visit_enter(ir_if *)
-   {
-      ++control_flow;
-      return visit_continue;
-   }
-
-   virtual ir_visitor_status visit_leave(ir_if *)
-   {
-      --control_flow;
-      return visit_continue;
-   }
-
-   virtual ir_visitor_status visit_enter(ir_loop *)
-   {
-      ++control_flow;
-      return visit_continue;
-   }
-
-   virtual ir_visitor_status visit_leave(ir_loop *)
-   {
-      --control_flow;
-      return visit_continue;
-   }
-
-   /* FINISHME: `switch` is not expressed at the IR level -- it's already
-    * been lowered to a mess of `if`s. We'll correctly disallow any use of
-    * barrier() in a conditional path within the switch, but not in a path
-    * which is always hit.
-    */
-
-   virtual ir_visitor_status visit_enter(ir_call *ir)
-   {
-      if (ir->use_builtin && strcmp(ir->callee_name(), "barrier") == 0) {
-         /* Use of barrier(); determine if it is legal: */
-         if (!in_main) {
-            linker_error(prog, "Builtin barrier() may only be used in main");
-            return visit_stop;
-         }
-
-         if (after_return) {
-            linker_error(prog, "Builtin barrier() may not be used after return");
-            return visit_stop;
-         }
-
-         if (control_flow != 0) {
-            linker_error(prog, "Builtin barrier() may not be used inside control flow");
-            return visit_stop;
-         }
-      }
-      return visit_continue;
-   }
-
-private:
-   gl_shader_program *prog;
-   bool in_main, after_return;
-   int control_flow;
-};
-
 /**
  * Visitor that determines the highest stream id to which a (geometry) shader
  * emits vertices. It also checks whether End{Stream}Primitive is ever called.
@@ -2355,14 +2264,6 @@ link_intrastage_shaders(void *mem_ctx,
    if (ctx->Const.VertexID_is_zero_based)
       lower_vertex_id(linked);
 
-   /* Validate correct usage of barrier() in the tess control shader */
-   if (linked->Stage == MESA_SHADER_TESS_CTRL) {
-      barrier_use_visitor visitor(prog);
-      foreach_in_list(ir_instruction, ir, linked->ir) {
-         ir->accept(&visitor);
-      }
-   }
-
    return linked;
 }