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

[Kenneth Graunke]

On Thursday, September 22, 2016 1:54:44 PM PDT Ian Romanick wrote:
> 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.

Yeah, that all works fine.  I thought it'd be a problem at first too.

The rules about function matching remain the same.  So, we'll find the
same function we found before.  Before this patch, we'd import a
prototype and tag it as "this is a built-in" or not.  The linker would
use that to link against the built-in or the user function.

Now, instead of emitting a tagged-prototype, we generate the built-in
inline.  That means that all prototypes are user functions.  The linker
doesn't need to think about built-ins anymore.

Works out surprisingly well.  Passes all the tests.

> 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?

I suppose not.  I thought "inline immediately" might be a reasonable
option for generate_call(), in case we wanted to use it in other cases.

Then again, I can't think of another case where we'd want it.

I'm happy to drop it.  What do you prefer?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160922/a51a353e/attachment-0001.sig>