[Mesa-dev] [PATCH 04/11] nir/spirv: add gl_spirv_validation method
Timothy Arceri
tarceri at itsqueeze.com
Fri Mar 16 11:14:20 UTC 2018
On 16/03/18 21:58, Alejandro Piñeiro wrote:
> On 16/03/18 11:47, Timothy Arceri wrote:
>> On 08/03/18 19:19, Alejandro Piñeiro 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;
>> Can you add a comment explaining what this is about? It just some
>> magic code to me.
>>
>> I saw this in the previous patch too. I assume we are just getting
>> past some sort of header, but maybe a define would be a good idea
>> since its done in multiple places.
> Yes, true, the meaning of that +5 got lost now that the comment "/*
> Handle the SPIR-V header (first 4 dwords) */" got moved to the
> vtn_create_builder function. How about something like:
>
> /* Skip the SPIR-V header, handled at vtn_create_builder */
> words+=5;
Sure sounds good. Thanks. I'll try to take a look at the rest of this
patch Monday. I'll leave patch 2 for Intel but a least you should be
able to push the core stuff if there are no issues here :)
More information about the mesa-dev
mailing list