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