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

[Ian Romanick]

On 09/22/2016 06:54 PM, Kenneth Graunke wrote:
> 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?

I was going to say I'd rather drop it, but I changed my mind.  I think
it's fine as is.

This patch is

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


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160923/1ff5d55c/attachment.sig>