<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
On 16/09/18 01:15, Alejandro Piñeiro wrote:<br>
<blockquote type="cite"
cite="mid:3a03c388-4cbf-978c-8247-333696e2ef58@igalia.com">
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
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>
</blockquote>
<br>
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. <br>
<br>
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.<br>
<pre>
</pre>
<blockquote type="cite"
cite="mid:3a03c388-4cbf-978c-8247-333696e2ef58@igalia.com"> <br>
<br>
[1]
<a class="moz-txt-link-freetext"
href="https://github.com/Igalia/piglit/commit/b5759c473861d7558a64e5bcacd45ba7b8c471b6"
moz-do-not-send="true">https://github.com/Igalia/piglit/commit/b5759c473861d7558a64e5bcacd45ba7b8c471b6</a><br>
[2]
<a class="moz-txt-link-freetext"
href="https://github.com/Igalia/piglit/commit/132825694b89a07f65b9452c3aaaa84b001f3733"
moz-do-not-send="true">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>
</blockquote>
<br>
</body>
</html>