Re: [Mesa-dev] [PATCH 17/24] mesa/main: Add a 'spirv' flag to gl_shader_program_data
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
Re: [Mesa-dev] [PATCH 17/24] mesa/main: Add a 'spirv' flag to gl_shader_program_data
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. 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). 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? 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
Re: [Mesa-dev] [PATCH 17/24] mesa/main: Add a 'spirv' flag to gl_shader_program_data
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
Re: [Mesa-dev] [PATCH 17/24] mesa/main: Add a 'spirv' flag to gl_shader_program_data
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. 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
Re: [Mesa-dev] [PATCH 17/24] mesa/main: Add a 'spirv' flag to gl_shader_program_data
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. 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
Re: [Mesa-dev] [PATCH 17/24] mesa/main: Add a 'spirv' flag to gl_shader_program_data
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. 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. 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
Re: [Mesa-dev] [PATCH 17/24] mesa/main: Add a 'spirv' flag to gl_shader_program_data
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 ... --- 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
[Mesa-dev] [PATCH 17/24] mesa/main: Add a 'spirv' flag to gl_shader_program_data
This will be used by the linker code to dfferentiate between programs made out of SPIR-V or GLSL shaders. --- 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; }; /** -- 2.11.0 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
