[Mesa-dev] [PATCH 02/26] glsl: Rebuild the symbol table without unreachable symbols

[Kenneth Graunke]

On Tuesday, July 15, 2014 08:05:31 PM Kenneth Graunke wrote:
> On Monday, July 14, 2014 03:48:34 PM Ian Romanick wrote:
> > From: Ian Romanick <ian.d.romanick at intel.com>
> > 
> > Previously we had to keep unreachable global symbols in the symbol table
> > because the symbol table is used during linking.  Having the symbol
> > table retain pointers to freed memory... what could possibly go wrong?
> > At the same time, this meant that we kept live references to tons of
> > memory that was no longer needed.
> > 
> > New strategy:  destroy the old symbol table, and make a new one from the
> > reachable symbols.
> > 
> > Valgrind massif results for a trimmed apitrace of dota2:
> > 
> >                   n        time(i)         total(B)   useful-heap(B) extra-
> heap(B)    stacks(B)
> > Before (32-bit): 59 40,642,425,451       76,337,968       69,720,886     
> 6,617,082            0
> > After  (32-bit): 46 40,661,487,174       75,116,800       68,854,065     
> 6,262,735            0
> > 
> > Before (64-bit): 79 37,179,441,771      106,986,512       98,112,095     
> 8,874,417            0
> > After  (64-bit): 64 37,200,329,700      104,872,672       96,514,546     
> 8,358,126            0
> > 
> > A real savings of 846KiB on 32-bit and 1.5MiB on 64-bit.
> > 
> > Signed-off-by: Ian Romanick <ian.d.romanick at intel.com>
> > Cc: Kenneth Graunke <kenneth at whitecape.org>
> > ---
> >  src/glsl/glsl_parser_extras.cpp | 37 ++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 36 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/glsl/glsl_parser_extras.cpp 
> b/src/glsl/glsl_parser_extras.cpp
> > index b327c2b..2962407 100644
> > --- a/src/glsl/glsl_parser_extras.cpp
> > +++ b/src/glsl/glsl_parser_extras.cpp
> > @@ -1485,7 +1485,7 @@ _mesa_glsl_compile_shader(struct gl_context *ctx, 
> struct gl_shader *shader,
> >     if (shader->InfoLog)
> >        ralloc_free(shader->InfoLog);
> >  
> > -   shader->symbols = state->symbols;
> > +   shader->symbols = new(shader->ir) glsl_symbol_table;
> >     shader->CompileStatus = !state->error;
> >     shader->InfoLog = state->info_log;
> >     shader->Version = state->language_version;
> > @@ -1498,6 +1498,41 @@ _mesa_glsl_compile_shader(struct gl_context *ctx, 
> struct gl_shader *shader,
> >     /* Retain any live IR, but trash the rest. */
> >     reparent_ir(shader->ir, shader->ir);
> >  
> > +   /* Destroy the symbol table.  Create a new symbol table that contains 
> only
> > +    * the variables and functions that still exist in the IR.  The symbol
> > +    * table will be used later during linking.
> > +    *
> > +    * There must NOT be any freed objects still referenced by the symbol
> > +    * table.  That could cause the linker to dereference freed memory.
> > +    *
> > +    * We don't have to worry about types or interface-types here because 
> those
> > +    * are fly-weights that are looked up by glsl_type.
> > +    */
> > +   foreach_in_list (ir_instruction, ir, shader->ir) {
> > +      switch (ir->ir_type) {
> > +      case ir_type_function: {
> > +         /* If the function is a built-in that is partially overridden in 
> the
> > +          * shader, the ir_function stored in the symbol table may not be 
> the
> > +          * same as the one that appears in the shader.  The one in the 
> shader
> > +          * will only include definitions from inside the shader.  We need 
> the
> > +          * one from the symbol table because it will include built-in
> > +          * defintions and definitions from the shader.
> > +          */
> > +         ir_function *const def = (ir_function *) ir;
> > +         ir_function *const decl = state->symbols->get_function(def->name);
> 
> Huh.  This doesn't match my understanding of the code.  I believe there 
> should only be one ir_function with a given name, and it should be both in the 
> shader's instruction stream and in the symbol table.
> 
> That single ir_function should contain any definitions and prototypes created 
> in the shader's GLSL code, and also prototypes (but not definitions) for any 
> built-in signatures called.
> 
> If you look at match_function_by_name(), it always uses the existing function, 
> if there is one, or creates a new one and adds it in both places...walking 
> through a couple scenarios, I haven't seen what's going wrong.
> 
> I suppose I should try the obvious
> 
>    shader->symobls->add_function((ir_function *) ir);
> 
> and see what breaks.  It sure doesn't seem like anything should.  I'll try to 
> look into it soon.

Okay, I finally got around to looking into this.  Doing the obvious approach breaks these two Piglit tests:

tests/spec/glsl-1.10/linker/override-builtin-{uniform,const}-05.shader_test

These have a single shader containing:
1. A function that calls built-in abs(float)
2. A user redefinition of abs(float)
3. A third call of abs() - which is supposed to be to be the user function.

At step 1, we create an ir_function for abs, and import the built-in signature.
At step 2 - when processing the function definition - we see that there is an existing ir_function, but ignore it as it only contains built-ins, and go create a second one.  This shadows the original in the symbol table; both get emitted into the IR stream.
At step 3, we use the symbol table to look up the ir_function, so we get the user defined one.

So you were right, and I forgot how this mess works.

That said, having two ir_functions in the IR stream with the same name seems really ugly to me.  I want there to be only one ir_function, and be able to find it either via the symbol table (if it's around), or a linked list walk if it isn't.  The fact that those two methods produce different results is...ugly.

I've sent out two patches to fix that, and rebased this one on top of it:
http://lists.freedesktop.org/archives/mesa-dev/2014-July/064052.html
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20140724/054d89ac/attachment-0001.sig>