[Mesa-dev] [PATCH] nir/spirv: add gl_spirv_validation method
Alejandro Piñeiro
apinheiro at igalia.com
Thu Jan 18 12:31:53 UTC 2018
On 18/01/18 08:35, Alejandro Piñeiro wrote:
> On 17/01/18 22:25, Jason Ekstrand wrote:
>> On Wed, Jan 17, 2018 at 3:31 AM, Alejandro Piñeiro
>> <apinheiro at igalia.com <mailto:apinheiro at igalia.com>> wrote:
>>
>> ARB_gl_spirv adds the ability to use SPIR-V binaries, and a new
>> method, glSpecializeShader. From OpenGL 4.6 spec, section 7.2.1
>> "Shader Specialization", error table:
>>
>> 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>.""
>>
>> But we are not really interested on creating the nir shader at that
>> point, and adding nir structures on the gl_program, so at that point
>> we are just interested on the error checking.
>>
>> So we add a new method focused on just checking those errors. It still
>> needs to parse the binary, but skips what it is not needed, and
>> doesn't create the nir shader.
>>
>> v2: rebase update (spirv_to_nir options added, changes on the warning
>> logging, and others)
>>
>> v3: include passing options on common initialization, doesn't call
>> setjmp on common_initialization
>>
>> v4: (after Jason comments):
>> * Rename common_initialization to vtn_builder_create
>> * Move validation method and their helpers to own source file.
>> * Create own handle_constant_decoration_cb instead of reuse
>> existing one
>> ---
>>
>>
>> I think that I handle all Jason concerns, although I have some minor
>> doubts:
>>
>> * Source file: I moved the validation method and his helpers to a new
>> source file. The name (compiler/spirv/gl_spirv.c) is debatable.
>>
>> * Headers: I didn't add a equivalent header, and I kept the
>> definition of the method at nir_spirv.h. I also put all the now
>> shared methods (like handle_decoration) to vtn_private. I hope
>> that's ok.
>>
>> * nir_spirv_specialization: I added the boolean "defined_on_module"
>> here, but this is only filled on gl_spirv_validation. And at the
>> same way, data32/data64 is only filled at spirv_to_nir. That sounds
>> somewhat strange. But at the same time adding a new struct
>> (something like gl_spirv_validation) with just the id and
>> "defined_on_module" seemed an overkill. I felt that reuse the
>> struct was the lesser evil. It is also debatable though.
>>
>>
>> src/compiler/Makefile.sources | 1 +
>> src/compiler/nir/meson.build | 1 +
>> src/compiler/spirv/gl_spirv.c | 268
>> ++++++++++++++++++++++++++++++++++++++
>> src/compiler/spirv/nir_spirv.h | 5 +
>> src/compiler/spirv/spirv_to_nir.c | 83 +++++++-----
>> src/compiler/spirv/vtn_private.h | 10 ++
>> 6 files changed, 338 insertions(+), 30 deletions(-)
>> create mode 100644 src/compiler/spirv/gl_spirv.c
>>
>> diff --git a/src/compiler/Makefile.sources
>> b/src/compiler/Makefile.sources
>> index d3f746f5f94..89c632112a2 100644
>> --- a/src/compiler/Makefile.sources
>> +++ b/src/compiler/Makefile.sources
>> @@ -294,6 +294,7 @@ SPIRV_GENERATED_FILES = \
>> spirv/vtn_gather_types.c
>>
>> SPIRV_FILES = \
>> + spirv/gl_spirv.c \
>> spirv/GLSL.std.450.h \
>> spirv/nir_spirv.h \
>> spirv/spirv.h \
>> diff --git a/src/compiler/nir/meson.build
>> b/src/compiler/nir/meson.build
>> index 5dd21e6652f..dd8cae59cc4 100644
>> --- a/src/compiler/nir/meson.build
>> +++ b/src/compiler/nir/meson.build
>> @@ -182,6 +182,7 @@ files_libnir = files(
>> 'nir_vla.h',
>> 'nir_worklist.c',
>> 'nir_worklist.h',
>> + '../spirv/gl_spirv.c',
>> '../spirv/GLSL.std.450.h',
>> '../spirv/nir_spirv.h',
>> '../spirv/spirv.h',
>> diff --git a/src/compiler/spirv/gl_spirv.c
>> b/src/compiler/spirv/gl_spirv.c
>> new file mode 100644
>> index 00000000000..e82686bfe0d
>> --- /dev/null
>> +++ b/src/compiler/spirv/gl_spirv.c
>> @@ -0,0 +1,268 @@
>> +/*
>> + * Copyright © 2017 Intel Corporation
>> + *
>> + * Permission is hereby granted, free of charge, to any person
>> obtaining a
>> + * copy of this software and associated documentation files (the
>> "Software"),
>> + * to deal in the Software without restriction, including without
>> limitation
>> + * the rights to use, copy, modify, merge, publish, distribute,
>> sublicense,
>> + * and/or sell copies of the Software, and to permit persons to
>> whom the
>> + * Software is furnished to do so, subject to the following
>> conditions:
>> + *
>> + * The above copyright notice and this permission notice
>> (including the next
>> + * paragraph) shall be included in all copies or substantial
>> portions of the
>> + * Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY
>> KIND, EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
>> MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO
>> EVENT SHALL
>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
>> DAMAGES OR OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
>> OTHERWISE, ARISING
>> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
>> OTHER DEALINGS
>> + * IN THE SOFTWARE.
>> + *
>> + *
>> + */
>> +
>> +#include "nir_spirv.h"
>> +
>> +#include "vtn_private.h"
>> +#include "spirv_info.h"
>> +
>> +static bool
>> +vtn_validate_preamble_instruction(struct vtn_builder *b, SpvOp
>> opcode,
>> + const uint32_t *w, unsigned count)
>>
>>
>> I think you could probably re-use all of
>> vtn_handle_preamble_instruction. It would do a bit more than strictly
>> needed (like handle capabilities) but I don't see any harm in it.
> Ok, will try to re-use it.
Well, I tried, and here the situation: as the validation is doing the
barely minimum to check for the errors defined at the method
glSpecializeShader, we are also passing it the barely minimum parameters
needed. So we are not passing spirv_to_nir_options. So if we try to
reuse vtn_handle_preamble_instruction during the validation, we start to
get several "Unsupported SPIR-V capabilities" vtn_warnings. So the
option is passing the spirv_to_nir_options here too, or just keep the
simplified version that this patch already includes.
>
>>
>>
>> +{
>> + switch (opcode) {
>> + case SpvOpSource:
>> + case SpvOpSourceExtension:
>> + case SpvOpSourceContinued:
>> + case SpvOpExtension:
>> + case SpvOpCapability:
>> + case SpvOpExtInstImport:
>> + case SpvOpMemoryModel:
>> + case SpvOpString:
>> + case SpvOpName:
>> + case SpvOpMemberName:
>> + case SpvOpExecutionMode:
>> + case SpvOpDecorationGroup:
>> + case SpvOpMemberDecorate:
>> + case SpvOpGroupDecorate:
>> + case SpvOpGroupMemberDecorate:
>> + break;
>> +
>> + case SpvOpEntryPoint:
>> + vtn_handle_entry_point(b, w, count);
>> + break;
>> +
>> + case SpvOpDecorate:
>> + vtn_handle_decoration(b, opcode, w, count);
>> + break;
>> +
>> + default:
>> + return false; /* End of preamble */
>> + }
>> +
>> + return true;
>> +}
>> +
>> +static void
>> +spec_constant_decoration_cb(struct vtn_builder *b, struct
>> vtn_value *v,
>> + int member, const struct
>> vtn_decoration *dec,
>> + void *data)
>> +{
>> + vtn_assert(member == -1);
>> + if (dec->decoration != SpvDecorationSpecId)
>> + return;
>> +
>> + for (unsigned i = 0; i < b->num_specializations; i++) {
>> + if (b->specializations[i].id == dec->literals[0]) {
>> + b->specializations[i].defined_on_module = true;
>>
>>
>> I see you setting this but I don't seeing it getting checked
>> anywhere. Perhaps something got lost in the refactor?
> It is used on the next patch "mesa: Implement glSpecializeShaderARB".
> That patch is the one that calls this method, and raise the gl errors if
> something went wrong. As those are specific opengl errors, I think that
> it is better to not raise them on source code at compiler/spirv.
>
>
>>
>>
>> + return;
>> + }
>> + }
>> +}
>> +
>> +static void
>> +vtn_validate_handle_constant(struct vtn_builder *b, SpvOp opcode,
>> + const uint32_t *w, unsigned count)
>> +{
>> + struct vtn_value *val = vtn_push_value(b, w[2],
>> vtn_value_type_constant);
>> +
>> + switch (opcode) {
>> + case SpvOpConstant:
>> + case SpvOpConstantNull:
>> + case SpvOpSpecConstantComposite:
>> + case SpvOpConstantComposite:
>> + /* Nothing to do here for gl_spirv needs */
>> + break;
>> +
>> + case SpvOpConstantTrue:
>> + case SpvOpConstantFalse:
>> + case SpvOpSpecConstantTrue:
>> + case SpvOpSpecConstantFalse:
>> + case SpvOpSpecConstant:
>> + case SpvOpSpecConstantOp:
>> + vtn_foreach_decoration(b, val, spec_constant_decoration_cb,
>> NULL);
>> + break;
>> +
>> + case SpvOpConstantSampler:
>> + vtn_fail("OpConstantSampler requires Kernel Capability");
>> + break;
>> +
>> + default:
>> + vtn_fail("Unhandled opcode");
>> + }
>> +}
>> +
>> +static bool
>> +vtn_validate_handle_constant_instruction(struct vtn_builder *b,
>> SpvOp opcode,
>> + const uint32_t *w,
>> unsigned count)
>> +{
>> + switch (opcode) {
>> + case SpvOpSource:
>> + case SpvOpSourceContinued:
>> + case SpvOpSourceExtension:
>> + case SpvOpExtension:
>> + case SpvOpCapability:
>> + case SpvOpExtInstImport:
>> + case SpvOpMemoryModel:
>> + case SpvOpEntryPoint:
>> + case SpvOpExecutionMode:
>> + case SpvOpString:
>> + case SpvOpName:
>> + case SpvOpMemberName:
>> + case SpvOpDecorationGroup:
>> + case SpvOpDecorate:
>> + case SpvOpMemberDecorate:
>> + case SpvOpGroupDecorate:
>> + case SpvOpGroupMemberDecorate:
>> + vtn_fail("Invalid opcode types and variables section");
>> + break;
>> +
>> + case SpvOpTypeVoid:
>> + case SpvOpTypeBool:
>> + case SpvOpTypeInt:
>> + case SpvOpTypeFloat:
>> + case SpvOpTypeVector:
>> + case SpvOpTypeMatrix:
>> + case SpvOpTypeImage:
>> + case SpvOpTypeSampler:
>> + case SpvOpTypeSampledImage:
>> + case SpvOpTypeArray:
>> + case SpvOpTypeRuntimeArray:
>> + case SpvOpTypeStruct:
>> + case SpvOpTypeOpaque:
>> + case SpvOpTypePointer:
>> + case SpvOpTypeFunction:
>> + case SpvOpTypeEvent:
>> + case SpvOpTypeDeviceEvent:
>> + case SpvOpTypeReserveId:
>> + case SpvOpTypeQueue:
>> + case SpvOpTypePipe:
>> + /* We don't need to handle types */
>> + break;
>> +
>> + case SpvOpConstantTrue:
>> + case SpvOpConstantFalse:
>> + case SpvOpConstant:
>> + case SpvOpConstantComposite:
>> + case SpvOpConstantSampler:
>> + case SpvOpConstantNull:
>> + case SpvOpSpecConstantTrue:
>> + case SpvOpSpecConstantFalse:
>> + case SpvOpSpecConstant:
>> + case SpvOpSpecConstantComposite:
>> + case SpvOpSpecConstantOp:
>> + vtn_validate_handle_constant(b, opcode, w, count);
>> + break;
>> +
>> + case SpvOpUndef:
>> + case SpvOpVariable:
>> + /* We don't need to handle them */
>> + break;
>> +
>> + default:
>> + return false; /* End of preamble */
>> + }
>> +
>> + return true;
>> +}
>> +
>> +/*
>> + * Since OpenGL 4.6 you can use SPIR-V modules directly on
>> OpenGL. One of the
>> + * new methods, glSpecializeShader include some possible errors
>> when trying to
>> + * use it. From OpenGL 4.6, Section 7.2.1, "Shader Specialization":
>> + *
>> + * "void SpecializeShaderARB(uint shader,
>> + * const char* pEntryPoint,
>> + * uint numSpecializationConstants,
>> + * const uint* pConstantIndex,
>> + * const uint* pConstantValue);
>> + * <skip>
>> + *
>> + * INVALID_VALUE is generated if <pEntryPoint> does not name a valid
>> + * entry point for <shader>.
>> + *
>> + * An INVALID_VALUE error is generated if any element of
>> pConstantIndex refers
>> + * to a specialization constant that does not exist in the shader
>> module
>> + * contained in shader."
>> + *
>> + * We could do those checks on spirv_to_nir, but we are only
>> interested on the
>> + * full translation later, during linking. This method is a
>> simplified version
>> + * of spirv_to_nir, looking for only the checks needed by
>> SpecializeShader.
>> + *
>> + * This method returns NULL if no entry point was found, and fill the
>> + * nir_spirv_specialization field "defined_on_module"
>> accordingly. Caller
>> + * would need to trigger the specific errors.
>> + *
>> + */
>> +bool
>> +gl_spirv_validation(const uint32_t *words, size_t word_count,
>> + struct nir_spirv_specialization *spec,
>> unsigned num_spec,
>> + gl_shader_stage stage, const char
>> *entry_point_name)
>> +{
>> + /* vtn_warn/vtn_log uses debug.func. Setting a null to prevent
>> crash. Not
>> + * need to print the warnings now, would be done later, on the
>> real
>> + * spirv_to_nir
>> + */
>> + const struct spirv_to_nir_options options = { .debug.func = NULL};
>> + const uint32_t *word_end = words + word_count;
>> +
>> + struct vtn_builder *b = vtn_builder_create(words, word_count,
>> + stage,
>> entry_point_name,
>> + &options);
>> +
>> + if (b == NULL)
>> + return false;
>> +
>> + /* See also _vtn_fail() */
>> + if (setjmp(b->fail_jump)) {
>> + ralloc_free(b);
>> + return false;
>> + }
>> +
>> + words+= 5;
>> +
>> + /* Search entry point from preamble */
>> + words = vtn_foreach_instruction(b, words, word_end,
>> +
>> vtn_validate_preamble_instruction);
>> +
>> + if (b->entry_point == NULL) {
>> + ralloc_free(b);
>> + return false;
>> + }
>> +
>> + b->specializations = spec;
>> + b->num_specializations = num_spec;
>> +
>> + /* Handle constant instructions (we don't need to handle
>> + * variables or types for gl_spirv)
>> + */
>> + words = vtn_foreach_instruction(b, words, word_end,
>> +
>> vtn_validate_handle_constant_instruction);
>> +
>> + ralloc_free(b);
>> +
>> + return true;
>> +}
>> +
>> diff --git a/src/compiler/spirv/nir_spirv.h
>> b/src/compiler/spirv/nir_spirv.h
>> index a2c40e57d18..d2766abb7f9 100644
>> --- a/src/compiler/spirv/nir_spirv.h
>> +++ b/src/compiler/spirv/nir_spirv.h
>> @@ -41,6 +41,7 @@ struct nir_spirv_specialization {
>> uint32_t data32;
>> uint64_t data64;
>> };
>> + bool defined_on_module;
>> };
>>
>> enum nir_spirv_debug_level {
>> @@ -69,6 +70,10 @@ struct spirv_to_nir_options {
>> } debug;
>> };
>>
>> +bool gl_spirv_validation(const uint32_t *words, size_t word_count,
>> + struct nir_spirv_specialization *spec,
>> unsigned num_spec,
>> + gl_shader_stage stage, const char
>> *entry_point_name);
>> +
>> nir_function *spirv_to_nir(const uint32_t *words, size_t word_count,
>> struct nir_spirv_specialization
>> *specializations,
>> unsigned num_specializations,
>> diff --git a/src/compiler/spirv/spirv_to_nir.c
>> b/src/compiler/spirv/spirv_to_nir.c
>> index c6df764682e..67b98fcb08d 100644
>> --- a/src/compiler/spirv/spirv_to_nir.c
>> +++ b/src/compiler/spirv/spirv_to_nir.c
>> @@ -458,7 +458,7 @@ vtn_foreach_execution_mode(struct vtn_builder
>> *b, struct vtn_value *value,
>> }
>> }
>>
>> -static void
>> +void
>> vtn_handle_decoration(struct vtn_builder *b, SpvOp opcode,
>> const uint32_t *w, unsigned count)
>> {
>> @@ -3065,6 +3065,24 @@ stage_for_execution_model(struct
>> vtn_builder *b, SpvExecutionModel model)
>> spirv_capability_to_string(cap)); \
>> } while(0)
>>
>> +
>> +void
>> +vtn_handle_entry_point(struct vtn_builder *b, const uint32_t *w,
>> + unsigned count)
>> +{
>> + struct vtn_value *entry_point = &b->values[w[2]];
>> + /* Let this be a name label regardless */
>> + unsigned name_words;
>> + entry_point->name = vtn_string_literal(b, &w[3], count - 3,
>> &name_words);
>> +
>> + if (strcmp(entry_point->name, b->entry_point_name) != 0 ||
>> + stage_for_execution_model(b, w[1]) != b->entry_point_stage)
>> + return;
>> +
>> + vtn_assert(b->entry_point == NULL);
>> + b->entry_point = entry_point;
>> +}
>> +
>> static bool
>> vtn_handle_preamble_instruction(struct vtn_builder *b, SpvOp opcode,
>> const uint32_t *w, unsigned count)
>> @@ -3219,20 +3237,9 @@ vtn_handle_preamble_instruction(struct
>> vtn_builder *b, SpvOp opcode,
>> w[2] == SpvMemoryModelGLSL450);
>> break;
>>
>> - case SpvOpEntryPoint: {
>> - struct vtn_value *entry_point = &b->values[w[2]];
>> - /* Let this be a name label regardless */
>> - unsigned name_words;
>> - entry_point->name = vtn_string_literal(b, &w[3], count - 3,
>> &name_words);
>> -
>> - if (strcmp(entry_point->name, b->entry_point_name) != 0 ||
>> - stage_for_execution_model(b, w[1]) != b->entry_point_stage)
>> - break;
>> -
>> - vtn_assert(b->entry_point == NULL);
>> - b->entry_point = entry_point;
>> + case SpvOpEntryPoint:
>> + vtn_handle_entry_point(b, w, count);
>> break;
>> - }
>>
>> case SpvOpString:
>> vtn_push_value(b, w[1], vtn_value_type_string)->str =
>> @@ -3775,12 +3782,10 @@ vtn_handle_body_instruction(struct
>> vtn_builder *b, SpvOp opcode,
>> return true;
>> }
>>
>> -nir_function *
>> -spirv_to_nir(const uint32_t *words, size_t word_count,
>> - struct nir_spirv_specialization *spec, unsigned
>> num_spec,
>> - gl_shader_stage stage, const char *entry_point_name,
>> - const struct spirv_to_nir_options *options,
>> - const nir_shader_compiler_options *nir_options)
>> +struct vtn_builder*
>> +vtn_builder_create(const uint32_t *words, size_t word_count,
>> + gl_shader_stage stage, const char
>> *entry_point_name,
>> + const struct spirv_to_nir_options *options)
>>
>>
>> Might be worth making this bit of refactoring its own patch. I don't
>> care that much though.
> As I would send a new version of the patch to reuse the preamble, I
> don't mind refactoring this.
Done locally. Will wait to agree on something related to the preamble
before sending it.
>
>>
>>
>> {
>> /* Initialize the stn_builder object */
>> struct vtn_builder *b = rzalloc(NULL, struct vtn_builder);
>> @@ -3794,14 +3799,6 @@ spirv_to_nir(const uint32_t *words, size_t
>> word_count,
>> b->entry_point_name = entry_point_name;
>> b->options = options;
>>
>> - /* See also _vtn_fail() */
>> - if (setjmp(b->fail_jump)) {
>> - ralloc_free(b);
>> - return NULL;
>> - }
>> -
>> - const uint32_t *word_end = words + word_count;
>> -
>> /* Handle the SPIR-V header (first 4 dwords) */
>> vtn_assert(word_count > 5);
>>
>> @@ -3811,11 +3808,37 @@ spirv_to_nir(const uint32_t *words, size_t
>> word_count,
>> unsigned value_id_bound = words[3];
>> vtn_assert(words[4] == 0);
>>
>> - words+= 5;
>> -
>> b->value_id_bound = value_id_bound;
>> b->values = rzalloc_array(b, struct vtn_value, value_id_bound);
>>
>> + return b;
>> +}
>> +
>> +nir_function *
>> +spirv_to_nir(const uint32_t *words, size_t word_count,
>> + struct nir_spirv_specialization *spec, unsigned
>> num_spec,
>> + gl_shader_stage stage, const char *entry_point_name,
>> + const struct spirv_to_nir_options *options,
>> + const nir_shader_compiler_options *nir_options)
>> +
>> +{
>> + const uint32_t *word_end = words + word_count;
>> +
>> + struct vtn_builder *b = vtn_builder_create(words, word_count,
>> + stage,
>> entry_point_name,
>> + options);
>> +
>> + if (b == NULL)
>> + return NULL;
>> +
>> + /* See also _vtn_fail() */
>> + if (setjmp(b->fail_jump)) {
>> + ralloc_free(b);
>> + return NULL;
>> + }
>> +
>> + words+= 5;
>> +
>> /* Handle all the preamble instructions */
>> words = vtn_foreach_instruction(b, words, word_end,
>> vtn_handle_preamble_instruction);
>> diff --git a/src/compiler/spirv/vtn_private.h
>> b/src/compiler/spirv/vtn_private.h
>> index 3e49df4dac8..04c59a160d0 100644
>> --- a/src/compiler/spirv/vtn_private.h
>> +++ b/src/compiler/spirv/vtn_private.h
>> @@ -710,6 +710,16 @@ void vtn_handle_alu(struct vtn_builder *b,
>> SpvOp opcode,
>> bool vtn_handle_glsl450_instruction(struct vtn_builder *b,
>> uint32_t ext_opcode,
>> const uint32_t *words,
>> unsigned count);
>>
>> +struct vtn_builder* vtn_builder_create(const uint32_t *words,
>> size_t word_count,
>> + gl_shader_stage stage,
>> const char *entry_point_name,
>> + const struct
>> spirv_to_nir_options *options);
>> +
>> +void vtn_handle_entry_point(struct vtn_builder *b, const uint32_t *w,
>> + unsigned count);
>> +
>> +void vtn_handle_decoration(struct vtn_builder *b, SpvOp opcode,
>> + const uint32_t *w, unsigned count);
>> +
>> static inline uint32_t
>> vtn_align_u32(uint32_t v, uint32_t a)
>> {
>> --
>> 2.11.0
>>
>>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
More information about the mesa-dev
mailing list