On 16/03/18 12:14, Timothy Arceri wrote: > > > 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 :) >
Thanks for the review. Really appreciated! BR _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev