<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
On 15/09/18 18:35, Jason Ekstrand wrote:<br>
<blockquote type="cite"
cite="mid:CAOFGe97giaMMx7v7cO3Nz+H1jXXovi15qWA0nZBwQoqcRU8iZA@mail.gmail.com">
<meta http-equiv="content-type" content="text/html; charset=utf-8">
<div dir="ltr">
<div class="gmail_quote">
<div dir="ltr">On Sat, Sep 15, 2018 at 11:19 AM Alejandro
Piñeiro <<a href="mailto:apinheiro@igalia.com"
moz-do-not-send="true">apinheiro@igalia.com</a>> wrote:<br>
</div>
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">Hi,<br>
<br>
this series adds the support for UBO and SSBO. The following
patches<br>
can be classified as:<br>
<br>
* Patches 1-8: changes on spirv to nir to take into account
ubo and<br>
ssbo, so it would be compatible to what OpenGL expects
(like having<br>
a interface_type, the correct mode, etc).<br>
<br>
* Patch 09: some nir wrappers over glsl_types methods to be
used<br>
later on the ARB_gl_spirv nir linker.<br>
<br>
* Patches 10-13: adding the explicit matrix stride, and the
explicit<br>
array stride on glsl_types, their nir C-wrappers, and the
proper<br>
filling up on the spirv to nir pass. This is needed
because on<br>
ARB_gl_spirv the layouts are gone, and are mapped with
explicits<br>
offsets/array strides/matrix strides. Spec quotes on
their<br>
patches. The array stride patch was somewhat more
intrusive that<br>
what I liked, but it was the best option we found.<br>
</blockquote>
</div>
</div>
</blockquote>
<br>
Hi, just a quick answer as it is Saturday night, we can elaborate
this Monday:<br>
<br>
<blockquote type="cite"
cite="mid:CAOFGe97giaMMx7v7cO3Nz+H1jXXovi15qWA0nZBwQoqcRU8iZA@mail.gmail.com">
<div dir="ltr">
<div class="gmail_quote">
<div><br>
</div>
<div>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. </div>
</div>
</div>
</blockquote>
<br>
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.<br>
<br>
<blockquote type="cite"
cite="mid:CAOFGe97giaMMx7v7cO3Nz+H1jXXovi15qWA0nZBwQoqcRU8iZA@mail.gmail.com">
<div dir="ltr">
<div class="gmail_quote">
<div>Does ARB_gl_spirv need it in NIR for introspection or
linking or something? </div>
</div>
</div>
</blockquote>
<br>
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.<br>
<br>
<blockquote type="cite"
cite="mid:CAOFGe97giaMMx7v7cO3Nz+H1jXXovi15qWA0nZBwQoqcRU8iZA@mail.gmail.com">
<div dir="ltr">
<div class="gmail_quote">
<div>I thought the introspection was all gone. </div>
</div>
</div>
</blockquote>
<br>
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.<br>
<br>
So, from ARB_gl_spirv spec, issue 8:<br>
<br>
<pre>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.</pre>
<br>
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.<br>
<br>
But at the same time, from issue 19:<br>
<pre>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>
</pre>
<br>
<pre> 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.</pre>
<br>
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.<br>
<br>
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).<br>
<br>
<br>
<blockquote type="cite"
cite="mid:CAOFGe97giaMMx7v7cO3Nz+H1jXXovi15qWA0nZBwQoqcRU8iZA@mail.gmail.com">
<div dir="ltr">
<div class="gmail_quote">
<div>It certainly doesn't need it to compute offsets because
we're already doing that in spirv_to_nir. <br>
</div>
</div>
</div>
</blockquote>
<br>
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).<br>
<br>
<blockquote type="cite"
cite="mid:CAOFGe97giaMMx7v7cO3Nz+H1jXXovi15qWA0nZBwQoqcRU8iZA@mail.gmail.com">
<div dir="ltr">
<div class="gmail_quote">
<div> What's so different about the way GL uses SPIR-V?</div>
</div>
</div>
</blockquote>
<br>
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.<br>
<br>
<br>
[1]
<a class="moz-txt-link-freetext" href="https://github.com/Igalia/piglit/commit/b5759c473861d7558a64e5bcacd45ba7b8c471b6">https://github.com/Igalia/piglit/commit/b5759c473861d7558a64e5bcacd45ba7b8c471b6</a><br>
[2]
<a class="moz-txt-link-freetext" href="https://github.com/Igalia/piglit/commit/132825694b89a07f65b9452c3aaaa84b001f3733">https://github.com/Igalia/piglit/commit/132825694b89a07f65b9452c3aaaa84b001f3733</a><br>
<br>
<blockquote type="cite"
cite="mid:CAOFGe97giaMMx7v7cO3Nz+H1jXXovi15qWA0nZBwQoqcRU8iZA@mail.gmail.com">
<div dir="ltr">
<div class="gmail_quote">
<div> </div>
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
* Patch 14: is_in_ubo/is_in_ssbo/is_in_block helpers on
nir.h,<br>
equivalent to the already existing at ir.h<br>
<br>
* Patches 15-16: lowering of vulkan_resource_index. Right
now the nir<br>
shader from spirv to nir includes this intrinsic. We
found more<br>
easy to lower it to something that OpenGL could
understand, instead<br>
of change spirv to nir to return one intrinsic or the
other,<br>
depending if you are on vulkan or opengl.<br>
<br>
* Patch 17: add the code to link blocks on the ARB_gl_spirv<br>
codepath. This became a kind of uber patch, but we found
more<br>
natural to keep in one commit, instead of split it. We
are open to<br>
suggestions.<br>
<br>
* Patches 18-22: updates on the uniform linking, now that
ubo/ssbos<br>
are supported.<br>
<br>
* Patch 23: add uniform blocks to the resource list.<br>
<br>
* Patches 24-25: call to the ARB_gl_spirv ubo/ssbo linking
method on<br>
the i965 driver, plus a nitpicky clean-up.<br>
<br>
* Patch 26: add some name NULL checks when querying<br>
NUM_ACTIVE_VARIABLES. We added several of such queries on
our<br>
tests, so it is somewhat unrelated nice to have.<br>
<br>
The tree for this series can be found on the following
repository:<br>
<a
href="https://github.com/Igalia/mesa/tree/arb_gl_spirv-series6-ubo-ssbo-v1"
rel="noreferrer" target="_blank" moz-do-not-send="true">https://github.com/Igalia/mesa/tree/arb_gl_spirv-series6-ubo-ssbo-v1</a><br>
<br>
And can be tested with the following piglit branch:<br>
<a
href="https://github.com/Igalia/piglit/tree/arb_gl_spirv-series5-ubo-ssbo-v1"
rel="noreferrer" target="_blank" moz-do-not-send="true">https://github.com/Igalia/piglit/tree/arb_gl_spirv-series5-ubo-ssbo-v1</a><br>
<br>
<br>
Alejandro Piñeiro (22):<br>
spirv/nir: translate uniform blocks<br>
spirv/nir: translate ssbo<br>
spirv/nir: setting interface type for ubos/ssbos<br>
spirv/nir: fill up nir variable info for ubos and ssbo<br>
spirv/nir: include SPIR-V explicit offset on the glsl
struct type<br>
spirv/nir: include row major coming from SPIR-V on the
glsl type<br>
spirv/nir: don't set interface_type if it is not a struct<br>
nir/types: add three new wrapper helpers<br>
glsl_types/nir: add matrix_stride plus nir wrapper helpers<br>
spirv/nir: fill glsl_struct_field explicit_matrix_stride<br>
glsl_types/nir: add explicit_array_stride plus nir wrapper
helpers<br>
spirv/nir: fill glsl_type array stride<br>
nir: add is_in_ubo/ssbo/block helpers<br>
nir/linker: add gl_nir_link_uniform_blocks.c<br>
nir/linker: fill is_shader_storage for uniforms<br>
nir/linker: use only the array element type for array of
ssbo/ubo<br>
nir/linker: fill up uniform_storage with explicit data<br>
nir/linker: update already processed uniforms search for
UBOs/SSBOs<br>
nir/linker: add program ubo/ssbo at the resource list<br>
i965: use GLboolean for all brw_link_shader returns<br>
i965: call to gl_nir_link_uniform_blocks<br>
mesa: add NULL name check for NUM_ACTIVE_VARIABLES query<br>
<br>
Antia Puentes (1):<br>
nir/linker: Set the uniform's block_index<br>
<br>
Neil Roberts (3):<br>
spirv/nir: Handle location decorations on block interface
members<br>
nir/linker/i965: Lower vulkan_resource_index during
linking<br>
nir/linker: handle non-ubo uses of vulkan_resource_index<br>
<br>
src/compiler/Makefile.sources | 2 +<br>
src/compiler/glsl/gl_nir.h | 4 +<br>
src/compiler/glsl/gl_nir_link_uniform_blocks.c | 713
+++++++++++++++++++++<br>
src/compiler/glsl/gl_nir_link_uniforms.c | 160
++++-<br>
src/compiler/glsl/gl_nir_linker.c | 14 +<br>
src/compiler/glsl/gl_nir_linker.h | 3 +<br>
src/compiler/glsl/gl_nir_lower_samplers_as_deref.c | 2 +-<br>
.../glsl/gl_nir_lower_vulkan_resource_index.c | 163
+++++<br>
src/compiler/glsl/meson.build | 2 +<br>
src/compiler/glsl_types.cpp | 31 +-<br>
src/compiler/glsl_types.h | 23 +-<br>
src/compiler/nir/nir.h | 22 +<br>
src/compiler/nir/nir_lower_io_arrays_to_elements.c | 3 +-<br>
src/compiler/nir/nir_split_per_member_structs.c | 3 +-<br>
src/compiler/nir/nir_split_vars.c | 3 +-<br>
src/compiler/nir_types.cpp | 47 +-<br>
src/compiler/nir_types.h | 20 +-<br>
src/compiler/spirv/spirv_to_nir.c | 24 +-<br>
src/compiler/spirv/vtn_private.h | 6 +<br>
src/compiler/spirv/vtn_variables.c | 90
++-<br>
src/mesa/drivers/dri/i965/brw_link.cpp | 12 +-<br>
src/mesa/main/shader_query.cpp | 30 +-<br>
22 files changed, 1301 insertions(+), 76 deletions(-)<br>
create mode 100644
src/compiler/glsl/gl_nir_link_uniform_blocks.c<br>
create mode 100644
src/compiler/glsl/gl_nir_lower_vulkan_resource_index.c<br>
<br>
-- <br>
2.14.1<br>
<br>
_______________________________________________<br>
mesa-dev mailing list<br>
<a href="mailto:mesa-dev@lists.freedesktop.org"
target="_blank" moz-do-not-send="true">mesa-dev@lists.freedesktop.org</a><br>
<a
href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev"
rel="noreferrer" target="_blank" moz-do-not-send="true">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a><br>
</blockquote>
</div>
</div>
</blockquote>
<br>
</body>
</html>