[Mesa-dev] [PATCH 2/4] glsl: Immediately inline built-ins rather than generating calls.

[Ian Romanick]

On 09/21/2016 10:20 PM, Kenneth Graunke wrote:
> In the past, we imported the prototypes of built-in functions, generated
> calls to those, and waited until link time to resolve the calls and
> import the actual code for the built-in functions.

I thought part of the reason we did this was to account for some of the
weird desktop GLSL rules about overriding and overloading built-in
functions.  Does this still handle that nonsense correctly?  I remember
you spent a lot of time rewritting this code to get all that right.

There's one additional question below.

> This severely limited our compile-time optimization opportunities: even
> trivial functions like dot() were represented as function calls.  We
> also had no way of reasoning about those calls; they could have been
> 1,000 line functions with side-effects for all we knew.
> 
> Practically all built-in functions are trivial translations to
> ir_expression opcodes, so it makes sense to just generate those inline.
> Since we eventually inline all functions anyway, we may as well just do
> it for all built-in functions.
> 
> There's only one snag: built-in functions that refer to built-in global
> variables need those remapped to the variables in the shader being
> compiled, rather than the ones in the built-in shader.  Currently,
> ftransform() is the only function matching those criteria, so it seemed
> easier to just make it a special case.
> 
> On Skylake:
> 
> total instructions in shared programs: 12023491 -> 12024010 (0.00%)
> instructions in affected programs: 77595 -> 78114 (0.67%)
> helped: 97
> HURT: 309
> 
> total cycles in shared programs: 137239044 -> 137295498 (0.04%)
> cycles in affected programs: 16714026 -> 16770480 (0.34%)
> helped: 4663
> HURT: 4923
> 
> while these statistics are in the wrong direction, the number of
> hurt programs is small (309 / 41282 = 0.75%), and I don't think
> anything can be done about it.  A change like this significantly
> alters the order in which optimizations are performed.
> 
> Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
> ---
>  src/compiler/glsl/ast_function.cpp | 46 ++++++++++++++++++--------------------
>  1 file changed, 22 insertions(+), 24 deletions(-)
> 
> diff --git a/src/compiler/glsl/ast_function.cpp b/src/compiler/glsl/ast_function.cpp
> index 7e62ab7..ac3b52d 100644
> --- a/src/compiler/glsl/ast_function.cpp
> +++ b/src/compiler/glsl/ast_function.cpp
> @@ -430,7 +430,8 @@ generate_call(exec_list *instructions, ir_function_signature *sig,
>                exec_list *actual_parameters,
>                ir_variable *sub_var,
>                ir_rvalue *array_idx,
> -              struct _mesa_glsl_parse_state *state)
> +              struct _mesa_glsl_parse_state *state,
> +              bool inline_immediately)

The caller just passes sig->is_builtin() for this parameter.  We already
pass sig into this function.  Do we need this new parameter?

>  {
>     void *ctx = state;
>     exec_list post_call_conversions;
> @@ -542,6 +543,10 @@ generate_call(exec_list *instructions, ir_function_signature *sig,
>     ir_call *call = new(ctx) ir_call(sig, deref,
>                                      actual_parameters, sub_var, array_idx);
>     instructions->push_tail(call);
> +   if (inline_immediately) {
> +      call->generate_inline(call);
> +      call->remove();
> +   }
>  
>     /* Also emit any necessary out-parameter conversions. */
>     instructions->append_list(&post_call_conversions);
> @@ -557,19 +562,18 @@ match_function_by_name(const char *name,
>                         exec_list *actual_parameters,
>                         struct _mesa_glsl_parse_state *state)
>  {
> -   void *ctx = state;
>     ir_function *f = state->symbols->get_function(name);
>     ir_function_signature *local_sig = NULL;
>     ir_function_signature *sig = NULL;
>  
>     /* Is the function hidden by a record type constructor? */
>     if (state->symbols->get_type(name))
> -      goto done; /* no match */
> +      return sig; /* no match */
>  
>     /* Is the function hidden by a variable (impossible in 1.10)? */
>     if (!state->symbols->separate_function_namespace
>         && state->symbols->get_variable(name))
> -      goto done; /* no match */
> +      return sig; /* no match */
>  
>     if (f != NULL) {
>        /* In desktop GL, the presence of a user-defined signature hides any
> @@ -583,31 +587,15 @@ match_function_by_name(const char *name,
>        sig = local_sig = f->matching_signature(state, actual_parameters,
>                                                allow_builtins, &is_exact);
>        if (is_exact)
> -         goto done;
> +         return sig;
>  
>        if (!allow_builtins)
> -         goto done;
> +         return sig;
>     }
>  
>     /* Local shader has no exact candidates; check the built-ins. */
>     _mesa_glsl_initialize_builtin_functions();
>     sig = _mesa_glsl_find_builtin_function(state, name, actual_parameters);
> -
> - done:
> -   if (sig != NULL) {
> -      /* If the match is from a linked built-in shader, import the
> -       * prototype.
> -       */
> -      if (sig != local_sig) {
> -         if (f == NULL) {
> -            f = new(ctx) ir_function(name);
> -            state->symbols->add_global_function(f);
> -            emit_function(state, f);
> -         }
> -         sig = sig->clone_prototype(f, NULL);
> -         f->add_signature(sig);
> -      }
> -   }
>     return sig;
>  }
>  
> @@ -2142,6 +2130,16 @@ ast_function_expression::hir(exec_list *instructions,
>                                           this->expressions)) {
>           /* an error has already been emitted */
>           value = ir_rvalue::error_value(ctx);
> +      } else if (sig->is_builtin() && strcmp(func_name, "ftransform") == 0) {
> +         /* ftransform refers to global variables, and we don't have any code
> +          * for remapping the variable references in the built-in shader.
> +          */
> +         ir_variable *mvp =
> +            state->symbols->get_variable("gl_ModelViewProjectionMatrix");
> +         ir_variable *vtx = state->symbols->get_variable("gl_Vertex");
> +         value = new(ctx) ir_expression(ir_binop_mul, glsl_type::vec4_type,
> +                                        new(ctx) ir_dereference_variable(mvp),
> +                                        new(ctx) ir_dereference_variable(vtx));
>        } else {
>           if (state->stage == MESA_SHADER_TESS_CTRL &&
>               sig->is_builtin() && strcmp(func_name, "barrier") == 0) {
> @@ -2162,8 +2160,8 @@ ast_function_expression::hir(exec_list *instructions,
>              }
>           }
>  
> -         value = generate_call(instructions, sig,
> -                               &actual_parameters, sub_var, array_idx, state);
> +         value = generate_call(instructions, sig, &actual_parameters, sub_var,
> +                               array_idx, state, sig->is_builtin());
>           if (!value) {
>              ir_variable *const tmp = new(ctx) ir_variable(glsl_type::void_type,
>                                                            "void_var",
>