[Mesa-dev] [PATCH 1/4] glsl: Check TCS barrier restrictions at ast_to_hir time, not link time.

[Ian Romanick]

This is much simpler.  This patch is

Reviewed-by: Ian Romanick <ian.d.romanick at intel.com>

On 09/21/2016 10:20 PM, Kenneth Graunke wrote:
> 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 at whitecape.org>
> ---
>  src/compiler/glsl/ast_function.cpp | 19 ++++++++
>  src/compiler/glsl/linker.cpp       | 99 --------------------------------------
>  2 files changed, 19 insertions(+), 99 deletions(-)
> 
> diff --git a/src/compiler/glsl/ast_function.cpp b/src/compiler/glsl/ast_function.cpp
> index ea3ac87..7e62ab7 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 f3eece2..606d006 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;
>  }
>  
>