On 17/01/18 22:25, Jason Ekstrand wrote:
> On Wed, Jan 17, 2018 at 3:31 AM, Alejandro Piñeiro
> <apinhe...@igalia.com <mailto:apinhe...@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.
>
>
> +{
> + 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.
>
>
> {
> /* 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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
