On 16/09/18 01:15, Alejandro Piñeiro wrote: > On 15/09/18 18:35, Jason Ekstrand wrote: >> On Sat, Sep 15, 2018 at 11:19 AM Alejandro Piñeiro >> <apinhe...@igalia.com <mailto:apinhe...@igalia.com>> wrote: >> >> Hi, >> >> this series adds the support for UBO and SSBO. The following patches >> can be classified as: >> >> * Patches 1-8: changes on spirv to nir to take into account ubo and >> ssbo, so it would be compatible to what OpenGL expects (like >> having >> a interface_type, the correct mode, etc). >> >> * Patch 09: some nir wrappers over glsl_types methods to be used >> later on the ARB_gl_spirv nir linker. >> >> * Patches 10-13: adding the explicit matrix stride, and the explicit >> array stride on glsl_types, their nir C-wrappers, and the proper >> filling up on the spirv to nir pass. This is needed because on >> ARB_gl_spirv the layouts are gone, and are mapped with explicits >> offsets/array strides/matrix strides. Spec quotes on their >> patches. The array stride patch was somewhat more intrusive that >> what I liked, but it was the best option we found. >> > > Hi, just a quick answer as it is Saturday night, we can elaborate this > Monday: > >> >> I'm very confused as to why all this is needed. We already have that >> information in vtn_type and not putting it in the glsl_type means >> that we get type equality when two types have the same basic >> structure even if it's not the same memory layout. > > Yes, that is something nice that we lose here. That is partly included > on the "the array stride was somewhat more intrusive that what I > liked". That feature is also gone when we include the matrix stride, > but in addition, array stride needs to touch a lot of places. I never > feel really good with that patch. > >> Does ARB_gl_spirv need it in NIR for introspection or linking or >> something? > > Yes, we are using it for the linking. This is mostly done on the big > patch 17, that is basically replicating what GLSL does with IR, trying > to fill the same structures, but with the NIR coming from the spirv to > nir pass. > >> I thought the introspection was all gone. > > Reading the spec, what we understood is that *new* > introspection/reflection was discarded, to be added later if needed, > or any reflection based on the name. > > So, from ARB_gl_spirv spec, issue 8: > > 8. Do we want to build a reflection API on SPIR-V modules specifics? > > - Retrieve compilation arguments > - Retrieve shader stages included > - Retrieve the list of entry points > - Retrieve the list of required capabilities > > Arguably, it's trivial to build a SPIR-V parser and figure this out > ourselves. > > DISCUSSION: > > If drivers implement SPIR-V support by lowering it to the same > representation that GLSL is lowered to, and that representation is the > source of reflection information, much reflection would naturally be > present. > > RESOLVED: First start without reflection, then add later if needed. > In the meantime, we have to document how the existing API operates > with no reflection information. See Issue 22. > > The RESOLVED part is somewhat ambiguous, as it refers only to > reflection that requires a name. That is the reason that it points to > issue 22., that explains what program interface queries and similar, > that uses names, should return if names are missing. > > But at the same time, from issue 19: > 19. How should the program interface query operations behave for program > objects created from SPIR-V shaders? > > DISCUSSION: we previously said we didn't need reflection to work > for SPIR-V shaders (at least for the first version), however we > are left with specifying how it should "not work". The primary issue > is that SPIR-V binaries are not required to have names associated > with variables. They can be associated in debug information, but there > is no requirement for that to be present, and it should not be relied > upon. > > <skipping options discarded> > > C) Allow as much as possible to work "naturally". You can query for > the number of active resources, and for details about them. Anything > that doesn't query by name will work as expected. Queries for maximum > length of names return one. Queries for anything "by name" return > INVALID_INDEX (or -1). Querying the name property of a resource > returns an empty string. This may allow many queries to work, but it's > not clear how useful it would be if you can't actually know which > specific variable you are retrieving information on. If everything > is specified a-priori by location/binding/offset/index/component > in the shader, this may be sufficient. > > RESOLVED. Pick (c), but also allow debug names to be returned if an > implementation wants to. > > They chose C), that points that any existing query that doesn't use > the name should work. It is true that it is somewhat confusing that > they say first that no reflection is needed to work now. Having said > so, the RESOLVED is clear when choosing C). In order to test C) our > piglit tests include several queries that doesn't require the name. We > even introduced a force-no-name mode for accessing ubos/ssbos [1], > that is basically do the same but using binding/location/etc. > > So for example, all our ubo piglit tests contains a query for > GL_BUFFER_DATA_SIZE, in order to return the ubo size. We compute that > size at link time, and we need the explicit offset/matrix stride/array > stride to be available (unless Im missing something). We even have a > test that use the same basic structure for two different ubos, but > different matrix stride [2], so different layout and a different > buffer size (and internally, two different glsl_types). > > >> It certainly doesn't need it to compute offsets because we're already >> doing that in spirv_to_nir. > > Yes, in order to run the shader, everything is there. In fact for a > long time the execution part of the shader (access to the ubo, with > different matrix stride if needed, etc) was working fine without those > changes. We needed to add those in order to compute the buffer size, > or any already existing query that doesn't require the name directly > (see C) again). > >> What's so different about the way GL uses SPIR-V? > > As far as I see, the idea behind ARB_gl_spirv is be able to use > SPIR-V, but behaving as similar as possible as using GLSL. Somewhat a > "let's not change how everything works, let's get working as much of > possible, so the transition would be easy". Somehow lets get the "best > of both worlds", but in the end, it doesn't really matter, with SPIR-V > you need to work without names, so you can't rely on them, so things > doesn't behave similar at all. Sometimes it looks more like the worst > of both worlds.
BTW, while rechecking the CTS tests for the extension, I realized that there is another proof that everything is expected to work as if you were using GLSL, except the name-based reflection. If you take a look to issue 22. of the spec, it list what the different name-based queries should return if the name is missing (NULL). But note that the value returned is different to what is should return if the specific variable/block/etc is not missing. So for example, a query for ACTIVE_UNIFORM_BLOCK_MAX_NAME_LENGTH should return 0 if there are no blocks, but 1 if there are blocks, but all block names are NULL (fwiw, there is a CTS test that specifically checks this). So that means that you need to populate ubos/ssbos at gl_shader_program_data, so you need a linking of the ubo/ssbos info at the NIR shader coming from the spirv to nir pass. This is also a somewhat-indirect proof that as much as possible is expected to work "naturally", as issue 19 pointed. So, again, that included a query for GL_BUFFER_DATA_SIZE. And yes, adding the explicit matrix/array strides on the glsl_type have some cons. I'm open to alternatives. > > > [1] > https://github.com/Igalia/piglit/commit/b5759c473861d7558a64e5bcacd45ba7b8c471b6 > [2] > https://github.com/Igalia/piglit/commit/132825694b89a07f65b9452c3aaaa84b001f3733 > >> >> >> * Patch 14: is_in_ubo/is_in_ssbo/is_in_block helpers on nir.h, >> equivalent to the already existing at ir.h >> >> * Patches 15-16: lowering of vulkan_resource_index. Right now >> the nir >> shader from spirv to nir includes this intrinsic. We found more >> easy to lower it to something that OpenGL could understand, >> instead >> of change spirv to nir to return one intrinsic or the other, >> depending if you are on vulkan or opengl. >> >> * Patch 17: add the code to link blocks on the ARB_gl_spirv >> codepath. This became a kind of uber patch, but we found more >> natural to keep in one commit, instead of split it. We are open to >> suggestions. >> >> * Patches 18-22: updates on the uniform linking, now that ubo/ssbos >> are supported. >> >> * Patch 23: add uniform blocks to the resource list. >> >> * Patches 24-25: call to the ARB_gl_spirv ubo/ssbo linking method on >> the i965 driver, plus a nitpicky clean-up. >> >> * Patch 26: add some name NULL checks when querying >> NUM_ACTIVE_VARIABLES. We added several of such queries on our >> tests, so it is somewhat unrelated nice to have. >> >> The tree for this series can be found on the following repository: >> >> https://github.com/Igalia/mesa/tree/arb_gl_spirv-series6-ubo-ssbo-v1 >> >> And can be tested with the following piglit branch: >> >> https://github.com/Igalia/piglit/tree/arb_gl_spirv-series5-ubo-ssbo-v1 >> >> >> Alejandro Piñeiro (22): >> spirv/nir: translate uniform blocks >> spirv/nir: translate ssbo >> spirv/nir: setting interface type for ubos/ssbos >> spirv/nir: fill up nir variable info for ubos and ssbo >> spirv/nir: include SPIR-V explicit offset on the glsl struct type >> spirv/nir: include row major coming from SPIR-V on the glsl type >> spirv/nir: don't set interface_type if it is not a struct >> nir/types: add three new wrapper helpers >> glsl_types/nir: add matrix_stride plus nir wrapper helpers >> spirv/nir: fill glsl_struct_field explicit_matrix_stride >> glsl_types/nir: add explicit_array_stride plus nir wrapper helpers >> spirv/nir: fill glsl_type array stride >> nir: add is_in_ubo/ssbo/block helpers >> nir/linker: add gl_nir_link_uniform_blocks.c >> nir/linker: fill is_shader_storage for uniforms >> nir/linker: use only the array element type for array of ssbo/ubo >> nir/linker: fill up uniform_storage with explicit data >> nir/linker: update already processed uniforms search for UBOs/SSBOs >> nir/linker: add program ubo/ssbo at the resource list >> i965: use GLboolean for all brw_link_shader returns >> i965: call to gl_nir_link_uniform_blocks >> mesa: add NULL name check for NUM_ACTIVE_VARIABLES query >> >> Antia Puentes (1): >> nir/linker: Set the uniform's block_index >> >> Neil Roberts (3): >> spirv/nir: Handle location decorations on block interface members >> nir/linker/i965: Lower vulkan_resource_index during linking >> nir/linker: handle non-ubo uses of vulkan_resource_index >> >> src/compiler/Makefile.sources | 2 + >> src/compiler/glsl/gl_nir.h | 4 + >> src/compiler/glsl/gl_nir_link_uniform_blocks.c | 713 >> +++++++++++++++++++++ >> src/compiler/glsl/gl_nir_link_uniforms.c | 160 ++++- >> src/compiler/glsl/gl_nir_linker.c | 14 + >> src/compiler/glsl/gl_nir_linker.h | 3 + >> src/compiler/glsl/gl_nir_lower_samplers_as_deref.c | 2 +- >> .../glsl/gl_nir_lower_vulkan_resource_index.c | 163 +++++ >> src/compiler/glsl/meson.build | 2 + >> src/compiler/glsl_types.cpp | 31 +- >> src/compiler/glsl_types.h | 23 +- >> src/compiler/nir/nir.h | 22 + >> src/compiler/nir/nir_lower_io_arrays_to_elements.c | 3 +- >> src/compiler/nir/nir_split_per_member_structs.c | 3 +- >> src/compiler/nir/nir_split_vars.c | 3 +- >> src/compiler/nir_types.cpp | 47 +- >> src/compiler/nir_types.h | 20 +- >> src/compiler/spirv/spirv_to_nir.c | 24 +- >> src/compiler/spirv/vtn_private.h | 6 + >> src/compiler/spirv/vtn_variables.c | 90 ++- >> src/mesa/drivers/dri/i965/brw_link.cpp | 12 +- >> src/mesa/main/shader_query.cpp | 30 +- >> 22 files changed, 1301 insertions(+), 76 deletions(-) >> create mode 100644 src/compiler/glsl/gl_nir_link_uniform_blocks.c >> create mode 100644 >> src/compiler/glsl/gl_nir_lower_vulkan_resource_index.c >> >> -- >> 2.14.1 >> >> _______________________________________________ >> mesa-dev mailing list >> mesa-dev@lists.freedesktop.org >> <mailto: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
