On 16/03/18 12:39, Rob Clark wrote:
> 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.
Yes, if that happens, I agree that it doesn't make sense to re-implement
spirv-val.
> 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
Out of curiosity: why do you expect that for mesa? FWIW, in the
short-medium term, we are going to send a proposal to piglit for the
ARB_gl_spirv testing, that would include add a spirv-tools dependency on
piglit. But using spirv-tools for the testing part sounds somewhat
straightforward to me. I don't see so obvious the need for mesa, as both
vulkan/ARB_gl_spirv allows a undefined behaviour for invalid SPIR-V.
Unless we are interested in a kind of "safe-mode" were the driver does a
full validation of the SPIR-V shader.
>
> 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
