On 21/11/17 21:30, Eduardo Lima Mitev wrote:
On 11/20/2017 01:54 PM, Timothy Arceri wrote:
On 20/11/17 21:56, Eduardo Lima Mitev wrote:
On 11/20/2017 11:31 AM, Timothy Arceri wrote:
On 20/11/17 19:00, Eduardo Lima Mitev wrote:
On 11/16/2017 12:23 AM, Timothy Arceri wrote:
On 16/11/17 00:22, Eduardo Lima Mitev wrote:
This will be used by the linker code to dfferentiate between
programs made out of SPIR-V or GLSL shaders.
So far everywhere this is used it seems you could just do
something like:
if (shProg->_LinkedShaders[stage]->spirv_data)
... spriv stuff ...
else
... glsl stuff ...
This flag is a per-program variable (as oppose to a per-shader
one). While it would be possible to know the type of program (spirv
vs. glsl) indirectly by looking at its linked shaders, I think it
is clearer and more readable (and more formal) to have a flag in
the program data for this.
The flag is per-program but as I said above everywhere you use it
you have access to the shader so it's totally unneeded. Adding bools
everywhere just ends up with two ways to get to the same solution
making things less clear and less readable because the code always
ends up using a bool here and a NULL check there. It's hardly a
stretch to see that if you have spirv_data than its a spriv shader.
Please check these two patches which are part of the larger,
in-progress work to support gl_spirv:
https://github.com/Igalia/mesa/commit/968c17dd4258af731bf14e5836e5ededced5d6f5
https://github.com/Igalia/mesa/commit/0bf39a1d815fdcb0ba281507a6f2d6d88a6dc1a1
These two explain why we need a a per-program flag to fork code-paths
between glsl and spirv. In both cases we are not in a per-shader
context, so we cannot practically use spirv_data from gl_linked_shaders.
Now, if you have a proposal for a different per-program way to do
what the above two patches intend, I will be happy to consider it.
Why do this for spriv only? Why not us the new nir linker for GLSL
also? There are many go reasons for doing so.
I agree there are good reasons to eventually use the NIR linker for both
SPIR-V and GLSL.
However, this should be a long term goal.
Having a NIR linker with the same level of completeness and quality as
the current GLSL linker requires a lot of effort, that will likely span
months. We certainly don't want to block supporting ARB_gl_spirv until
we have a regression-free linker for both SPIR-V and GLSL programs.
So far you have uniform and resource var linking functions. I guess
UBO's could complicate things but it really feels like these should be
converted as a preparation series rather than having to come back and
add additional support later on. It may take a little longer but it
seems worth it IMO.
Having this temporary fork in the driver linking code-path allows us to
introduce the NIR linker incrementally, activating it on only for a
subset of cases (SPIR-V programs).
I'm sorry but I really don't see the advantage of doing this. In the end
I guess it's up to you how you do it.
We have also experimented with activating Ian's GLSL-to-SPIR-V generator
under an environment variable. This will take normal GLSL shaders and
turn them into SPIR-V shaders, then route them through our SPIR-V paths.
This could be an
intermediary step before going full NIR linker.
Timothy, what about if I drop this patch from this series, and
re-introduced it in the next series when the code-path forks really need
the flag? Would that be ok for you?
Seems like a good idea for now.
Eduardo
It is my fault if I didn't explain in the commit log that the flag
would be needed in the 2nd series. Or better, I should have moved
this patch to the 2nd series where it is actually used.
Eduardo
As the person that spent a bunch of time cleaning up all these
structs at then end of last year (I believe I landed around 100
patches to clean up the mess) I'm very much against adding
unnecessary fields back in here.
This also allows for releasing the memory of the spirv_data
structure when we don't need it, and still being able to know what
kind of program we have.
Why do you need this though? The backend shouldn't care where it
came from once you have compiled it right? You also can't free it
until gl_shader is freed in which case the API can't query it
anymore either so as far as I can see it's not needed.
I would personally keep this flag.
Are there more opinions about this?
Thanks for reviewing, Timothy.
Edu
---
src/mesa/main/mtypes.h | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/src/mesa/main/mtypes.h b/src/mesa/main/mtypes.h
index d624f2cbd19..db9c2e1deaa 100644
--- a/src/mesa/main/mtypes.h
+++ b/src/mesa/main/mtypes.h
@@ -2902,6 +2902,12 @@ struct gl_shader_program_data
/* Mask of stages this program was linked against */
unsigned linked_stages;
+
+ /* Whether the shaders of this program are loaded from SPIR-V
binaries
+ * (all have the SPIR_V_BINARY_ARB state). This was
introduced by the
+ * ARB_gl_spirv extension.
+ */
+ bool spirv;
};
/**
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
