[Mesa-dev] [PATCH 00/26] ARB_gl_spirv: ubo/ssbo support
Alejandro Piñeiro
apinheiro at igalia.com
Sat Sep 15 23:15:44 UTC 2018
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.
[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/20180916/d7083b2f/attachment-0001.html>
More information about the mesa-dev
mailing list