[Mesa-dev] [PATCH 21/24] mesa/glspirv: Create gl_linked_shader objects for a SPIR-V program

[Timothy Arceri]

On 16/11/17 00:22, Eduardo Lima Mitev wrote:
> ---
>   src/mesa/main/glspirv.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++---
>   1 file changed, 48 insertions(+), 3 deletions(-)
> 
> diff --git a/src/mesa/main/glspirv.c b/src/mesa/main/glspirv.c
> index 86f1d221cc9..9332764f152 100644
> --- a/src/mesa/main/glspirv.c
> +++ b/src/mesa/main/glspirv.c
> @@ -29,6 +29,8 @@
>   #include "compiler/nir/nir.h"
>   #include "compiler/spirv/nir_spirv.h"
>   
> +#include "program/program.h"
> +
>   #include "util/u_atomic.h"
>   
>   void
> @@ -117,14 +119,57 @@ _mesa_spirv_shader_binary(struct gl_context *ctx,
>      }
>   }
>   
> +/**
> + * This is the equivalent to compiler/glsl/linker.cpp::link_shaders()
> + * but for SPIR-V programs.
> + *
> + * This method just creates the gl_linked_shader structs with a reference to
> + * the SPIR-V data collected during previous steps.
> + *
> + * The real linking happens later in the driver-specifc call LinkShader().
> + * This is so backends can implement different linking strategies for
> + * SPIR-V programs.
> + */
>   void
>   _mesa_spirv_link_shaders(struct gl_context *ctx, struct gl_shader_program *prog)
>   {
> -   /* @TODO: This is a placeholder for the equivalent of
> -    * compiler/glsl/linker.cpp::link_shaders() but for SPIR-V.
> -    */
>      prog->data->LinkStatus = linking_success;
>      prog->data->Validated = false;
> +
> +   for (unsigned i = 0; i < prog->NumShaders; i++) {


The GL api allows multiple shaders to be attached to the same stage of a 
program. This code assumes a 1-to-1 mapping of shaders to stages.

I don't see anything in the spec that says its an error to attach more 
than one shader to a single stage, but this looks like a probable and 
might require a spec bug to be filed.

With this existing code we will leak gl_linked_shader and never unref 
spirv_data should multiple shaders be attached to the same stage.


> +      struct gl_shader *shader = prog->Shaders[i];
> +      gl_shader_stage shader_type = shader->Stage;
> +
> +      assert(shader->spirv_data);
> +
> +      struct gl_linked_shader *linked = rzalloc(NULL, struct gl_linked_shader);
> +      linked->Stage = shader_type;
> +
> +      /* Create program and attach it to the linked shader */
> +      struct gl_program *gl_prog =
> +         ctx->Driver.NewProgram(ctx,
> +                                _mesa_shader_stage_to_program(shader_type),
> +                                prog->Name, false);
> +      if (!gl_prog) {
> +         prog->data->LinkStatus = linking_failure;
> +         _mesa_delete_linked_shader(ctx, linked);
> +         return;
> +      }
> +
> +      _mesa_reference_shader_program_data(ctx,
> +                                          &gl_prog->sh.data,
> +                                          prog->data);
> +
> +      /* Don't use _mesa_reference_program() just take ownership */
> +      linked->Program = gl_prog;
> +
> +      /* Reference the SPIR-V data from shader to the linked shader */
> +      _mesa_shader_spirv_data_reference(&linked->spirv_data,
> +                                        shader->spirv_data);
> +
> +      prog->_LinkedShaders[shader_type] = linked;
> +      prog->data->linked_stages |= 1 << shader_type;
> +   }
>   }
>   
>   void GLAPIENTRY
>