On 16/03/18 12:11, Rob Clark wrote:
> Just curious, how much different is this from what spirv-val does?
Well, spirv-val does a full validation of the SPIR-V binary. Here we are
just doing the basic validation required by the method
glSpecializeShadeARB, introduced by ARB_gl_spirv. That's the reason the
method is called gl_spirv_validation and not spirv_validation.
Specifically the minimum enough to be able to raise the following
possible errors from that method:
"INVALID_VALUE is generated if <pEntryPoint> does not match the
<Name> of any OpEntryPoint declaration in the SPIR-V module
associated with <shader>.
INVALID_OPERATION is generated if the <Execution Mode> of the
OpEntryPoint indicated by <pEntryPoint> does not match the type
of <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>."
So plenty of invalid SPIR-V binaries would be able to pass that point.
And that is "okish" to not check that. Also from 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."
> We
> are going to end up using spirv-tools from clover, so if all of this
> is just to avoid a dependency on spirv-tools, is it worth it?
As I mentioned, this is basically because we don't need so such deep
validation. Perhaps the name is slightly confusing even with the gl_
prefix. In any case, note that with this patch we add a big comment
(almost a test wall) for this method that explain what it is really
validating.
>
> BR,
> -R
>
> On Thu, Mar 8, 2018 at 3:19 AM, Alejandro Piñeiro <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
>>
>> v5: put vtn_build_create refactoring to their own patch (Jason)
>> ---
>> 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 | 35 +++--
>> src/compiler/spirv/vtn_private.h | 6 +
>> 6 files changed, 302 insertions(+), 14 deletions(-)
>> create mode 100644 src/compiler/spirv/gl_spirv.c
>>
>> diff --git a/src/compiler/Makefile.sources b/src/compiler/Makefile.sources
>> index 37340ba809e..24218985d6d 100644
>> --- a/src/compiler/Makefile.sources
>> +++ b/src/compiler/Makefile.sources
>> @@ -295,6 +295,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 a70c236b958..e7a94bcc1cf 100644
>> --- a/src/compiler/nir/meson.build
>> +++ b/src/compiler/nir/meson.build
>> @@ -183,6 +183,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)
>> +{
>> + 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;
>> + 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 2b0bdaec013..87d4120c380 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 {
>> @@ -70,6 +71,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 fb63df209ce..66b87c049bb 100644
>> --- a/src/compiler/spirv/spirv_to_nir.c
>> +++ b/src/compiler/spirv/spirv_to_nir.c
>> @@ -461,7 +461,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)
>> {
>> @@ -3164,6 +3164,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)
>> @@ -3352,20 +3370,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 =
>> diff --git a/src/compiler/spirv/vtn_private.h
>> b/src/compiler/spirv/vtn_private.h
>> index a0934158df8..2219f4619dc 100644
>> --- a/src/compiler/spirv/vtn_private.h
>> +++ b/src/compiler/spirv/vtn_private.h
>> @@ -723,6 +723,12 @@ 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.14.1
>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
