[Mesa-dev] [PATCH] i965: Free dead GLSL IR one last time.

[Ian Romanick]

On 04/02/2015 10:55 AM, Thomas Helland wrote:
> This reminds me of a patch Eric wrote that probably
> fell through the cracks when he migrated jobs:
> 
> https://freedesktop.org/patch/26778/
> "Free the compiled shader IR after it has been linked"

There's a pretty significant bug triggered by that code.  If you have a
shader that sets uses a uniform initializer like

uniform bool use_slow_method = false;

then change that uniform via glUniform*, then trigger a recompile, the
value will get reset to the in-shader value.

It's quite a chain of events, but, IIRC, there was *one* piglit test
that hit this.  It shouldn't be too hard to fix.  I think we'd just need
a flag to the linker for it to ignore uniform initializers on the
re-compile pass.

> It got an R-B, but seems like it never made it to upstream.
> I don't know if it still applies to the way things are now though.
> There might have been a discussion on IRC that concluded
> that it should be dropped? I don't know.
> It still has status "New" in patchwork, so seems unlikely.
> Seems like it should be sort of a big deal, so thought I'd ping it.
> 
> Regards,
> Thomas
> 
> 2015-04-02 11:04 GMT+02:00 Kenneth Graunke <kenneth at whitecape.org>:
>> While working on NIR's memory allocation model, I realized the GLSL IR
>> memory model was broken.
>>
>> During glCompileShader, we allocate everything out of the
>> _mesa_glsl_parse_state context, and reparent it to gl_shader at the end.
>>
>> During glLinkProgram, we allocate everything out of a temporary context,
>> then reparent it to the exec_list containing the linked IR.
>>
>> But during brw_link_shader - the driver's final opportunity to do
>> lowering and optimization - we just allocated everything out of the
>> permanent context given to us by the linker.  That memory stayed
>> forever.
>>
>> Notably, passes like brw_fs_channel_expressions cause us to churn the
>> majority of the code, so we really want to free dead IR here.
>>
>> Saves 125MB of memory when replaying a Dota 2 trace on Broadwell.
>>
>> Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
>> ---
>>  src/mesa/drivers/dri/i965/brw_shader.cpp | 12 ++++++++++++
>>  1 file changed, 12 insertions(+)
>>
>> This depends on my new ralloc_adopt() API, proposed here:
>> http://lists.freedesktop.org/archives/mesa-dev/2015-March/080763.html
>>
>> XXX: need to confirm piglit results (I kicked off tests, but am waiting for results)
>> XXX: should probably Cc stable.
>>
>> diff --git a/src/mesa/drivers/dri/i965/brw_shader.cpp b/src/mesa/drivers/dri/i965/brw_shader.cpp
>> index 0dda9bb..b923e54 100644
>> --- a/src/mesa/drivers/dri/i965/brw_shader.cpp
>> +++ b/src/mesa/drivers/dri/i965/brw_shader.cpp
>> @@ -144,6 +144,11 @@ brw_link_shader(struct gl_context *ctx, struct gl_shader_program *shProg)
>>
>>        _mesa_copy_linked_program_data((gl_shader_stage) stage, shProg, prog);
>>
>> +      /* Temporary memory context for any new IR. */
>> +      void *mem_ctx = ralloc_context(NULL);
>> +
>> +      ralloc_adopt(mem_ctx, shader->base.ir);
>> +
>>        bool progress;
>>
>>        /* lower_packing_builtins() inserts arithmetic instructions, so it
>> @@ -249,6 +254,13 @@ brw_link_shader(struct gl_context *ctx, struct gl_shader_program *shProg)
>>
>>        _mesa_reference_program(ctx, &prog, NULL);
>>
>> +      /* Now that we've finished altering the linked IR, reparent any live IR back
>> +       * to the permanent memory context, and free the temporary one (discarding any
>> +       * junk we optimized away).
>> +       */
>> +      reparent_ir(shader->base.ir, shader->base.ir);
>> +      ralloc_free(mem_ctx);
>> +
>>        if (ctx->_Shader->Flags & GLSL_DUMP) {
>>           fprintf(stderr, "\n");
>>           fprintf(stderr, "GLSL IR for linked %s program %d:\n",
>> --
>> 2.3.4
>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>