[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.
>
> 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 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