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

[Eduardo Lima Mitev]

On 11/27/2017 03:20 AM, Timothy Arceri wrote:
> 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.
>

Yes. I had a comment saying that we only support one shader per stage,
but it got removed at some point.

> 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.
>

Yes, I think the extension spec should forbid it. As the API is designed
right now, all shaders attached to the program must have been
specialized, and specialization requires specifying an entry
pointfunction name. Now, what happens if there are more that one shader
for a single stage? What's the valid entry point? Or should all shaders
of a stage have been specialized with the same entry point? This needs
clarificationIMO.

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

Right, good catch.I will update the patch to put back the comment saying
that for now we only support one shader per stage, and also bail with a
linker error if we see a second shader for a given stage.

Thanks, Timothy.

Eduardo

>
>> +      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
>>
>