[Mesa-dev] [PATCH 00/26] ARB_gl_spirv: ubo/ssbo support

Alejandro Piñeiro apinheiro at igalia.com
Tue Sep 18 10:52:17 UTC 2018

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
>> <apinheiro at igalia.com <mailto:apinheiro at 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.
>     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 at lists.freedesktop.org
>>     <mailto:mesa-dev at lists.freedesktop.org>
>>     https://lists.freedesktop.org/mailman/listinfo/mesa-dev

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180918/10995d18/attachment-0001.html>

More information about the mesa-dev mailing list