[Mesa-dev] [PATCH 11/70] glsl: create gl_program at the start of linking rather than the end

[Emil Velikov]

On 11 November 2016 at 00:45, Timothy Arceri
<timothy.arceri at collabora.com> wrote:
> This will allow us to directly store metadata we want to retain in
> gl_program this metadata is currently stored in gl_linked_shader and
> will be lost if relinking fails even though the program will remain
> in use and is still valid according to the spec.
>
> "If a program object that is active for any shader stage is re-linked
> unsuccessfully, the link status will be set to FALSE, but any existing
> executables and associated state will remain part of the current
> rendering state until a subsequent call to UseProgram,
> UseProgramStages, or BindProgramPipeline removes them from use."
>
> This change will also help avoid the double handing that happens in
> _mesa_copy_linked_program_data().
> ---
>  src/compiler/glsl/linker.cpp               | 15 +++++++++++++++
>  src/mesa/drivers/dri/i965/brw_link.cpp     |  9 +--------
>  src/mesa/program/ir_to_mesa.cpp            |  6 +-----
>  src/mesa/state_tracker/st_glsl_to_nir.cpp  |  7 +------
>  src/mesa/state_tracker/st_glsl_to_tgsi.cpp |  8 ++------
>  5 files changed, 20 insertions(+), 25 deletions(-)
>
> diff --git a/src/compiler/glsl/linker.cpp b/src/compiler/glsl/linker.cpp
> index 693a50b..f63c025 100644
> --- a/src/compiler/glsl/linker.cpp
> +++ b/src/compiler/glsl/linker.cpp
> @@ -72,6 +72,7 @@
>  #include "ir.h"
>  #include "program.h"
>  #include "program/prog_instruction.h"
> +#include "program/program.h"
>  #include "util/set.h"
>  #include "util/string_to_uint_map.h"
>  #include "linker.h"
> @@ -2183,6 +2184,20 @@ link_intrastage_shaders(void *mem_ctx,
>     }
>
>     gl_linked_shader *linked = ctx->Driver.NewShader(shader_list[0]->Stage);
> +
> +   /* Create program and attach it to the linked shader */
> +   struct gl_program *gl_prog =
> +      ctx->Driver.NewProgram(ctx,
> +                             _mesa_shader_stage_to_program(shader_list[0]->Stage),
> +                             prog->Name);
> +   if (!prog) {
> +      prog->LinkStatus = false;
> +      _mesa_delete_linked_shader(ctx, linked);
> +      return NULL;
> +   }
> +
> +   _mesa_reference_program(ctx, &linked->Program, gl_prog);
> +
I'm not too sure referencing seems right in this patch.
All the error paths seem to be missing the deref, is that intentional
or a bug ? I'm leaning toward the latter.

>     linked->ir = new(linked) exec_list;
>     clone_ir_list(mem_ctx, linked->ir, main->ir);
>
> diff --git a/src/mesa/drivers/dri/i965/brw_link.cpp b/src/mesa/drivers/dri/i965/brw_link.cpp
> index 248d2f1..d2c8ed3 100644
> --- a/src/mesa/drivers/dri/i965/brw_link.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_link.cpp
> @@ -221,14 +221,7 @@ brw_link_shader(struct gl_context *ctx, struct gl_shader_program *shProg)
>        if (!shader)
>           continue;
>
> -      struct gl_program *prog =
> -         ctx->Driver.NewProgram(ctx, _mesa_shader_stage_to_program(stage),
> -                                0);
> -      if (!prog)
> -        return false;
> -
> -      _mesa_reference_program(ctx, &shader->Program, prog);
> -
> +      struct gl_program *prog = shader->Program;
>        prog->Parameters = _mesa_new_parameter_list();
>
>        process_glsl_ir(brw, shProg, shader);
> diff --git a/src/mesa/program/ir_to_mesa.cpp b/src/mesa/program/ir_to_mesa.cpp
> index e24fb50b..75234d7 100644
> --- a/src/mesa/program/ir_to_mesa.cpp
> +++ b/src/mesa/program/ir_to_mesa.cpp
> @@ -2791,9 +2791,7 @@ get_mesa_program(struct gl_context *ctx,
>
>     validate_ir_tree(shader->ir);
>
> -   prog = ctx->Driver.NewProgram(ctx, target, shader_program->Name);
> -   if (!prog)
> -      return NULL;
> +   prog = shader->Program;
>     prog->Parameters = _mesa_new_parameter_list();
>     v.ctx = ctx;
>     v.prog = prog;
> @@ -2927,8 +2925,6 @@ get_mesa_program(struct gl_context *ctx,
>        prog->info.fs.depth_layout = shader_program->FragDepthLayout;
>     }
>
> -   _mesa_reference_program(ctx, &shader->Program, prog);
> -
>     if ((ctx->_Shader->Flags & GLSL_NO_OPT) == 0) {
>        _mesa_optimize_program(ctx, prog, prog);
>     }
> diff --git a/src/mesa/state_tracker/st_glsl_to_nir.cpp b/src/mesa/state_tracker/st_glsl_to_nir.cpp
> index 36531ad..cbc7e5a 100644
> --- a/src/mesa/state_tracker/st_glsl_to_nir.cpp
> +++ b/src/mesa/state_tracker/st_glsl_to_nir.cpp
> @@ -367,15 +367,10 @@ st_nir_get_mesa_program(struct gl_context *ctx,
>                          struct gl_linked_shader *shader)
>  {
>     struct gl_program *prog;
> -   GLenum target = _mesa_shader_stage_to_program(shader->Stage);
>
>     validate_ir_tree(shader->ir);
>
> -   prog = ctx->Driver.NewProgram(ctx, target, shader_program->Name);
> -   if (!prog)
> -      return NULL;
> -
> -   _mesa_reference_program(ctx, &shader->Program, prog);
> +   prog = shader->Program;
>
>     prog->Parameters = _mesa_new_parameter_list();
>
> diff --git a/src/mesa/state_tracker/st_glsl_to_tgsi.cpp b/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
> index 7a4ee36..c430e1a 100644
> --- a/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
> +++ b/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
> @@ -6407,7 +6407,6 @@ get_mesa_program_tgsi(struct gl_context *ctx,
>  {
>     glsl_to_tgsi_visitor* v;
>     struct gl_program *prog;
> -   GLenum target = _mesa_shader_stage_to_program(shader->Stage);
>     struct gl_shader_compiler_options *options =
>           &ctx->Const.ShaderCompilerOptions[shader->Stage];
>     struct pipe_screen *pscreen = ctx->st->pipe->screen;
> @@ -6415,11 +6414,7 @@ get_mesa_program_tgsi(struct gl_context *ctx,
>
>     validate_ir_tree(shader->ir);
>
> -   prog = ctx->Driver.NewProgram(ctx, target, shader_program->Name);
> -   if (!prog)
> -      return NULL;
> -
> -   _mesa_reference_program(ctx, &shader->Program, prog);
> +   prog = shader->Program;
>
>     prog->Parameters = _mesa_new_parameter_list();
>     v = new glsl_to_tgsi_visitor();
> @@ -6540,6 +6535,7 @@ get_mesa_program_tgsi(struct gl_context *ctx,
>      */
>     _mesa_associate_uniform_storage(ctx, shader_program, prog->Parameters);
>     if (!shader_program->LinkStatus) {
> +      _mesa_reference_program(ctx, &shader->Program, NULL);
>        free_glsl_to_tgsi_visitor(v);
>        return NULL;
>     }
Mildly (if at all?) related comment: worth adding the above/similar
hunk to st_glsl_to_nir.cpp:st_nir_get_mesa_program or there's
something subtle happening there. In which case worth adding a comment
?
In either case that's something which can be addressed separately.

With the extra deref(s) on error in link_intrastage_shaders (assuming
I haven't lost my marbles), this and 10/70 are
Reviewed-by: Emil Velikov <emil.velikov at collabora.com>

-Emil