On Fri, Mar 16, 2018 at 7:27 AM, Alejandro Piñeiro <apinhe...@igalia.com> wrote:
> 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."
ok, if the spec doesn't require full validation and this doesn't
eventually evolve into a re-implementation of spirv-val, I'm not
really against it. But just figured I should point out that I expect
eventually distro mesa builds will anyways depend on spirv-tools, so
if that was an easier path I don't see a big issue with a dependency
on spirv-tools
BR,
-R
>> 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
