<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Wed, Jan 17, 2018 at 3:31 AM, Alejandro Piñeiro <<a href="mailto:apinheiro@igalia.com" target="_blank">apinheiro@igalia.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">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 --- I think that I handle all Jason concerns, although I have some minor doubts: * Source file: I moved the validation method and his helpers to a new source file. The name (compiler/spirv/gl_spirv.c) is debatable. * Headers: I didn't add a equivalent header, and I kept the definition of the method at nir_spirv.h. I also put all the now shared methods (like handle_decoration) to vtn_private. I hope that's ok. * nir_spirv_specialization: I added the boolean "defined_on_module" here, but this is only filled on gl_spirv_validation. And at the same way, data32/data64 is only filled at spirv_to_nir. That sounds somewhat strange. But at the same time adding a new struct (something like gl_spirv_validation) with just the id and "defined_on_module" seemed an overkill. I felt that reuse the struct was the lesser evil. It is also debatable though. 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 | 83 +++++++----- src/compiler/spirv/vtn_private.h | 10 ++ 6 files changed, 338 insertions(+), 30 deletions(-) create mode 100644 src/compiler/spirv/gl_spirv.c diff --git a/src/compiler/Makefile.sources b/src/compiler/Makefile.sources index d3f746f5f94..89c632112a2 100644 --- a/src/compiler/Makefile.sources +++ b/src/compiler/Makefile.sources @@ -294,6 +294,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 5dd21e6652f..dd8cae59cc4 100644 --- a/src/compiler/nir/meson.build +++ b/src/compiler/nir/meson.build @@ -182,6 +182,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) </blockquote><div> </div><div>I think you could probably re-use all of vtn_handle_preamble_instruction. It would do a bit more than strictly needed (like handle capabilities) but I don't see any harm in it. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> +{ + 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; </blockquote><div> </div><div>I see you setting this but I don't seeing it getting checked anywhere. Perhaps something got lost in the refactor? </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> + 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 a2c40e57d18..d2766abb7f9 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 { @@ -69,6 +70,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 c6df764682e..67b98fcb08d 100644 --- a/src/compiler/spirv/spirv_to_nir.c +++ b/src/compiler/spirv/spirv_to_nir.c @@ -458,7 +458,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) { @@ -3065,6 +3065,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) @@ -3219,20 +3237,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 = @@ -3775,12 +3782,10 @@ vtn_handle_body_instruction(struct vtn_builder *b, SpvOp opcode, return true; } -nir_function * -spirv_to_nir(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, - const struct spirv_to_nir_options *options, - const nir_shader_compiler_options *nir_options) +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) </blockquote><div> </div><div>Might be worth making this bit of refactoring its own patch. I don't care that much though. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> { /* Initialize the stn_builder object */ struct vtn_builder *b = rzalloc(NULL, struct vtn_builder); @@ -3794,14 +3799,6 @@ spirv_to_nir(const uint32_t *words, size_t word_count, b->entry_point_name = entry_point_name; b->options = options; - /* See also _vtn_fail() */ - if (setjmp(b->fail_jump)) { - ralloc_free(b); - return NULL; - } - - const uint32_t *word_end = words + word_count; - /* Handle the SPIR-V header (first 4 dwords) */ vtn_assert(word_count > 5); @@ -3811,11 +3808,37 @@ spirv_to_nir(const uint32_t *words, size_t word_count, unsigned value_id_bound = words[3]; vtn_assert(words[4] == 0); - words+= 5; - b->value_id_bound = value_id_bound; b->values = rzalloc_array(b, struct vtn_value, value_id_bound); + return b; +} + +nir_function * +spirv_to_nir(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, + const struct spirv_to_nir_options *options, + const nir_shader_compiler_options *nir_options) + +{ + 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 NULL; + + /* See also _vtn_fail() */ + if (setjmp(b->fail_jump)) { + ralloc_free(b); + return NULL; + } + + words+= 5; + /* Handle all the preamble instructions */ words = vtn_foreach_instruction(b, words, word_end, vtn_handle_preamble_instruction); diff --git a/src/compiler/spirv/vtn_private.h b/src/compiler/spirv/vtn_private.h index 3e49df4dac8..04c59a160d0 100644 --- a/src/compiler/spirv/vtn_private.h +++ b/src/compiler/spirv/vtn_private.h @@ -710,6 +710,16 @@ void vtn_handle_alu(struct vtn_builder *b, SpvOp opcode, bool vtn_handle_glsl450_instruction(struct vtn_builder *b, uint32_t ext_opcode, const uint32_t *words, unsigned count); +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.11.0 </blockquote></div> </div></div>