[Mesa-dev] [PATCH 16/24] mesa: Implement glSpecializeShaderARB

[Eduardo Lima]

On 11/27/2017 11:06 AM, Alejandro Piñeiro wrote:
> On 27/11/17 03:22, Timothy Arceri wrote:
>> I suspect this patch doesn't compile. I think pEntryPoint is meant to
>> be passed as an argument to this function.

pEntryPoint is passed as an argument. What makes you think it is not?

> It is the second parameter of the method. But it is true that it doesn't
> appear on this patch. Seems that the diff skipped some lines there.

It doesn't appear in the hunk below because git is showing just 3 lines 
of context above and below each hunk. pEntryPoint arg simply falls outside.

> In any case, note that the method was added on patch "mesa: add
> GL_ARB_gl_spirv boilerplate", with pEntryPoint as second parameter, and
> no other patch is changing that.
>
>> On 16/11/17 00:22, Eduardo Lima Mitev wrote:
>>> From: Nicolai Hähnle <nicolai.haehnle at amd.com>
>>>
>>> v2: use gl_spirv_validation instead of spirv_to_nir.
>>>      This method just validates the shader. The conversion to NIR will
>>>      happen later, during linking. (Alejandro Piñeiro)
>>>
>>> v3: Use gl_shader_spirv_data struct to store the SPIR-V data.
>>>      (Eduardo Lima)
>>> ---
>>>    src/mesa/main/glspirv.c | 107
>>> ++++++++++++++++++++++++++++++++++++++++++++++++
>>>    1 file changed, 107 insertions(+)
>>>
>>> diff --git a/src/mesa/main/glspirv.c b/src/mesa/main/glspirv.c
>>> index 390fa9e6984..fe954b05c7b 100644
>>> --- a/src/mesa/main/glspirv.c
>>> +++ b/src/mesa/main/glspirv.c
>>> @@ -24,6 +24,10 @@
>>>    #include "glspirv.h"
>>>      #include "errors.h"
>>> +#include "shaderobj.h"
>>> +
>>> +#include "compiler/nir/nir.h"
>>> +#include "compiler/spirv/nir_spirv.h"
>>>      #include "util/u_atomic.h"
>>>    @@ -120,4 +124,107 @@ _mesa_SpecializeShaderARB(GLuint shader,
>>>                              const GLuint *pConstantIndex,
>>>                              const GLuint *pConstantValue)
>>>    {
>>> +   GET_CURRENT_CONTEXT(ctx);
>>> +   struct gl_shader *sh;
>>> +   bool has_entry_point;
>>> +   struct nir_spirv_specialization *spec_entries = NULL;
>>> +
>>> +   if (!ctx->Extensions.ARB_gl_spirv) {
>>> +      _mesa_error(ctx, GL_INVALID_OPERATION, "glSpecializeShaderARB");
>>> +      return;
>>> +   }
>>> +
>>> +   sh = _mesa_lookup_shader_err(ctx, shader, "glSpecializeShaderARB");
>>> +   if (!sh)
>>> +      return;
>>> +
>>> +   if (!sh->SpirVBinary) {
>>> +      _mesa_error(ctx, GL_INVALID_OPERATION,
>>> +                  "glSpecializeShaderARB(not SPIR-V)");
>>> +      return;
>>> +   }
>>> +
>>> +   if (sh->CompileStatus) {
>>> +      _mesa_error(ctx, GL_INVALID_OPERATION,
>>> +                  "glSpecializeShaderARB(already specialized)");
>>> +      return;
>>> +   }
>>> +
>>> +   struct gl_shader_spirv_data *spirv_data = sh->spirv_data;
>>> +   assert(spirv_data);
>>> +
>>> +   /* From the GL_ARB_gl_spirv spec:
>>> +    *
>>> +    *    "The OpenGL API expects the SPIR-V module to have already been
>>> +    *     validated, and can return an error if it discovers
>>> anything invalid
>>> +    *     in the module. An invalid SPIR-V module is allowed to
>>> result in
>>> +    *     undefined behavior."
>>> +    *
>>> +    * However, the following errors still need to be detected (from
>>> the same
>>> +    * spec):
>>> +    *
>>> +    *    "INVALID_VALUE is generated if <pEntryPoint> does not name
>>> a valid
>>> +    *     entry point for <shader>.
>>> +    *
>>> +    *     INVALID_VALUE is generated if any element of <pConstantIndex>
>>> +    *     refers to a specialization constant that does not exist in
>>> the
>>> +    *     shader module contained in <shader>."
>>> +    *
>>> +    * We cannot flag those errors a-priori because detecting them
>>> requires
>>> +    * parsing the module. However, flagging them during
>>> specialization is okay,
>>> +    * since it makes no difference in terms of application-visible
>>> state.
>>> +    */
>>> +   spec_entries = calloc(sizeof(*spec_entries),
>>> numSpecializationConstants);
>>> +
>>> +   for (unsigned i = 0; i < numSpecializationConstants; ++i) {
>>> +      spec_entries[i].id = pConstantIndex[i];
>>> +      spec_entries[i].data32 = pConstantValue[i];
>>> +      spec_entries[i].defined_on_module = false;
>>> +   }
>>> +
>>> +   has_entry_point =
>>> +      gl_spirv_validation((uint32_t
>>> *)&spirv_data->SpirVModule->Binary[0],
>>> +                          spirv_data->SpirVModule->Length / 4,
>>> +                          spec_entries, numSpecializationConstants,
>>> +                          sh->Stage, pEntryPoint);
>>> +
>>> +   /* See previous spec comment */
>>> +   if (!has_entry_point) {
>>> +      _mesa_error(ctx, GL_INVALID_VALUE,
>>> +                  "glSpecializeShaderARB(\"%s\" is not a valid entry
>>> point"
>>> +                  " for shader)", pEntryPoint);
>>> +      goto end;
>>> +   }
>>> +
>>> +   for (unsigned i = 0; i < numSpecializationConstants; ++i) {
>>> +      if (spec_entries[i].defined_on_module == false) {
>>> +         _mesa_error(ctx, GL_INVALID_VALUE,
>>> +                     "glSpecializeShaderARB(constant \"%i\" does not
>>> exist "
>>> +                     "in shader)", spec_entries[i].id);
>>> +         goto end;
>>> +      }
>>> +   }
>>> +
>>> +   spirv_data->SpirVEntryPoint = ralloc_strdup(spirv_data,
>>> pEntryPoint);
>>> +
>>> +   /* Note that we didn't make a real compilation of the module
>>> (spirv_to_nir),
>>> +    * but just checked some error conditions. Real "compilation"
>>> will be done
>>> +    * later, upon linking.
>>> +    */
>>> +   sh->CompileStatus = compile_success;
>>> +
>>> +   spirv_data->NumSpecializationConstants = numSpecializationConstants;
>>> +   spirv_data->SpecializationConstantsIndex =
>>> +      rzalloc_array_size(spirv_data, sizeof(GLuint),
>>> +                         numSpecializationConstants);
>>> +   spirv_data->SpecializationConstantsValue =
>>> +      rzalloc_array_size(spirv_data, sizeof(GLuint),
>>> +                         numSpecializationConstants);
>>> +   for (unsigned i = 0; i < numSpecializationConstants; ++i) {
>>> +      spirv_data->SpecializationConstantsIndex[i] = pConstantIndex[i];
>>> +      spirv_data->SpecializationConstantsValue[i] = pConstantValue[i];
>>> +   }
>>> +
>>> + end:
>>> +   free(spec_entries);
>>>    }
>>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>