[Mesa-dev] [PATCH 5/7] glsl/mesa: split gl_shader in two
Iago Toral
itoral at igalia.com
Wed Jun 29 12:36:14 UTC 2016
On Tue, 2016-06-28 at 11:52 +1000, Timothy Arceri wrote:
> There are two distinctly different uses of this struct. The first
> is to store GL shader objects. The second is to store information
> about a shader stage thats been linked.
>
> The two uses actually share few fields and there is clearly confusion
> about their use. For example the linked shaders map one to one with
> a program so can simply be destroyed along with the program. However
> previously we were calling reference counting on the linked shaders.
>
> We were also creating linked shaders with a name even though it
> is always 0 and called the driver version of the _mesa_new_shader()
> function unnecessarily for GL shader objects.
> ---
> src/compiler/glsl/glsl_to_nir.cpp | 2 +-
> src/compiler/glsl/ir_optimization.h | 23 ++--
> src/compiler/glsl/link_atomics.cpp | 2 +-
> src/compiler/glsl/link_functions.cpp | 8 +-
> src/compiler/glsl/link_interface_blocks.cpp | 10 +-
> src/compiler/glsl/link_uniform_blocks.cpp | 2 +-
> src/compiler/glsl/link_uniform_initializers.cpp | 6 +-
> src/compiler/glsl/link_uniforms.cpp | 8 +-
> src/compiler/glsl/link_varyings.cpp | 17 ++-
> src/compiler/glsl/link_varyings.h | 17 ++-
> src/compiler/glsl/linker.cpp | 106 +++++++--------
> src/compiler/glsl/linker.h | 10 +-
> src/compiler/glsl/lower_distance.cpp | 3 +-
> src/compiler/glsl/lower_named_interface_blocks.cpp | 2 +-
> src/compiler/glsl/lower_packed_varyings.cpp | 6 +-
> src/compiler/glsl/lower_shared_reference.cpp | 6 +-
> src/compiler/glsl/lower_tess_level.cpp | 2 +-
> src/compiler/glsl/lower_ubo_reference.cpp | 6 +-
> src/compiler/glsl/lower_vector_derefs.cpp | 2 +-
> src/compiler/glsl/lower_vertex_id.cpp | 2 +-
> src/compiler/glsl/opt_dead_builtin_varyings.cpp | 11 +-
> src/compiler/glsl/standalone.cpp | 4 +-
> src/compiler/glsl/standalone_scaffolding.cpp | 20 +++
> src/compiler/glsl/standalone_scaffolding.h | 7 +
> src/compiler/glsl/test_optpass.cpp | 2 +-
> src/mesa/drivers/common/meta.c | 2 +-
> src/mesa/drivers/dri/i965/brw_context.c | 5 +-
> src/mesa/drivers/dri/i965/brw_context.h | 8 +-
> src/mesa/drivers/dri/i965/brw_gs.c | 5 +-
> src/mesa/drivers/dri/i965/brw_link.cpp | 24 ++--
> src/mesa/drivers/dri/i965/brw_program.c | 2 +-
> src/mesa/drivers/dri/i965/brw_program.h | 2 +-
> src/mesa/drivers/dri/i965/brw_shader.cpp | 4 +-
> src/mesa/drivers/dri/i965/brw_shader.h | 3 +-
> src/mesa/drivers/dri/i965/brw_tcs.c | 2 +-
> src/mesa/drivers/dri/i965/brw_tes.c | 3 +-
> src/mesa/drivers/dri/i965/brw_wm_surface_state.c | 6 +-
> src/mesa/main/api_validate.c | 6 +-
> src/mesa/main/dd.h | 3 +-
> src/mesa/main/ff_fragment_shader.cpp | 2 +-
> src/mesa/main/mtypes.h | 143 ++++++++++++++++++---
> src/mesa/main/shader_query.cpp | 2 +-
> src/mesa/main/shaderapi.c | 19 +--
> src/mesa/main/shaderobj.c | 32 ++++-
> src/mesa/main/shaderobj.h | 7 +
> src/mesa/main/uniform_query.cpp | 4 +-
> src/mesa/main/uniforms.c | 2 +-
> src/mesa/program/ir_to_mesa.cpp | 4 +-
> src/mesa/program/ir_to_mesa.h | 2 +-
> src/mesa/program/prog_print.c | 14 +-
> src/mesa/program/prog_print.h | 2 +-
> src/mesa/state_tracker/st_atom_constbuf.c | 2 +-
> src/mesa/state_tracker/st_atom_image.c | 2 +-
> src/mesa/state_tracker/st_atom_storagebuf.c | 2 +-
> src/mesa/state_tracker/st_glsl_to_nir.cpp | 2 +-
> src/mesa/state_tracker/st_glsl_to_tgsi.cpp | 6 +-
> src/mesa/state_tracker/st_nir.h | 2 +-
> src/mesa/state_tracker/st_program.c | 7 -
> 58 files changed, 388 insertions(+), 227 deletions(-)
(...)
> diff --git a/src/compiler/glsl/link_interface_blocks.cpp b/src/compiler/glsl/link_interface_blocks.cpp
> index 5858eee..447d9a4 100644
> --- a/src/compiler/glsl/link_interface_blocks.cpp
> +++ b/src/compiler/glsl/link_interface_blocks.cpp
> @@ -343,8 +343,8 @@ validate_intrastage_interface_blocks(struct gl_shader_program *prog,
>
> void
> validate_interstage_inout_blocks(struct gl_shader_program *prog,
> - const gl_shader *producer,
> - const gl_shader *consumer)
> + const gl_linked_shader *producer,
> + const gl_linked_shader *consumer)
> {
> interface_block_definitions definitions;
> /* VS -> GS, VS -> TCS, VS -> TES, TES -> GS */
> @@ -384,15 +384,15 @@ validate_interstage_inout_blocks(struct gl_shader_program *prog,
>
> void
> validate_interstage_uniform_blocks(struct gl_shader_program *prog,
> - gl_shader **stages, int num_stages)
> + gl_linked_shader **stages)
> {
> interface_block_definitions definitions;
>
> - for (int i = 0; i < num_stages; i++) {
> + for (int i = 0; i < MESA_SHADER_STAGES; i++) {
> if (stages[i] == NULL)
> continue;
The two changes above look like they should go into a separate commit
(together with the equivalent change where we call
validate_interstage_uniform_blocks in linker.cpp
> - const gl_shader *stage = stages[i];
> + const gl_linked_shader *stage = stages[i];
> foreach_in_list(ir_instruction, node, stage->ir) {
> ir_variable *var = node->as_variable();
> if (!var || !var->get_interface_type() ||
(...)
> diff --git a/src/mesa/program/prog_print.c b/src/mesa/program/prog_print.c
> index 755d644..b8d7cca 100644
> --- a/src/mesa/program/prog_print.c
> +++ b/src/mesa/program/prog_print.c
> @@ -994,16 +994,6 @@ _mesa_write_shader_to_file(const struct gl_shader *shader)
> if (shader->InfoLog) {
> fputs(shader->InfoLog, f);
> }
> - if (shader->CompileStatus && shader->Program) {
> - fprintf(f, "/* GPU code */\n");
> - fprintf(f, "/*\n");
> - _mesa_fprint_program_opt(f, shader->Program, PROG_PRINT_DEBUG, GL_TRUE);
> - fprintf(f, "*/\n");
> - fprintf(f, "/* Parameters / constants */\n");
> - fprintf(f, "/*\n");
> - _mesa_fprint_parameter_list(f, shader->Program->Parameters);
> - fprintf(f, "*/\n");
> - }
I' am not sure where we use this, maybe for the shader-db scripts? Did
you check that this did not break anything there?
> fclose(f);
> }
> @@ -1015,7 +1005,7 @@ _mesa_write_shader_to_file(const struct gl_shader *shader)
> * _mesa_write_shader_to_file function.
> */
> void
> -_mesa_append_uniforms_to_file(const struct gl_shader *shader)
> +_mesa_append_uniforms_to_file(const struct gl_linked_shader *shader)
> {
> const struct gl_program *const prog = shader->Program;
> const char *type;
> @@ -1027,7 +1017,7 @@ _mesa_append_uniforms_to_file(const struct gl_shader *shader)
> else
> type = "vert";
>
> - _mesa_snprintf(filename, sizeof(filename), "shader_%u.%s", shader->Name, type);
> + _mesa_snprintf(filename, sizeof(filename), "shader.%s", type);
Same question here.
Iago.
> f = fopen(filename, "a"); /* append */
> if (!f) {
> fprintf(stderr, "Unable to open %s for appending\n", filename);
> diff --git a/src/mesa/program/prog_print.h b/src/mesa/program/prog_print.h
> index 9058dfa..7b1e1fe 100644
> --- a/src/mesa/program/prog_print.h
> +++ b/src/mesa/program/prog_print.h
> @@ -118,7 +118,7 @@ extern void
> _mesa_write_shader_to_file(const struct gl_shader *shader);
>
> extern void
> -_mesa_append_uniforms_to_file(const struct gl_shader *shader);
> +_mesa_append_uniforms_to_file(const struct gl_linked_shader *shader);
>
>
> #ifdef __cplusplus
> diff --git a/src/mesa/state_tracker/st_atom_constbuf.c b/src/mesa/state_tracker/st_atom_constbuf.c
> index a980dbe..594db1e 100644
> --- a/src/mesa/state_tracker/st_atom_constbuf.c
> +++ b/src/mesa/state_tracker/st_atom_constbuf.c
> @@ -265,7 +265,7 @@ const struct st_tracked_state st_update_cs_constants = {
> };
>
> static void st_bind_ubos(struct st_context *st,
> - struct gl_shader *shader,
> + struct gl_linked_shader *shader,
> unsigned shader_type)
> {
> unsigned i;
> diff --git a/src/mesa/state_tracker/st_atom_image.c b/src/mesa/state_tracker/st_atom_image.c
> index f8a0044..bc9344e 100644
> --- a/src/mesa/state_tracker/st_atom_image.c
> +++ b/src/mesa/state_tracker/st_atom_image.c
> @@ -45,7 +45,7 @@
> #include "st_format.h"
>
> static void
> -st_bind_images(struct st_context *st, struct gl_shader *shader,
> +st_bind_images(struct st_context *st, struct gl_linked_shader *shader,
> unsigned shader_type)
> {
> unsigned i;
> diff --git a/src/mesa/state_tracker/st_atom_storagebuf.c b/src/mesa/state_tracker/st_atom_storagebuf.c
> index 37b4c4d..0f96e6d 100644
> --- a/src/mesa/state_tracker/st_atom_storagebuf.c
> +++ b/src/mesa/state_tracker/st_atom_storagebuf.c
> @@ -41,7 +41,7 @@
> #include "st_program.h"
>
> static void
> -st_bind_ssbos(struct st_context *st, struct gl_shader *shader,
> +st_bind_ssbos(struct st_context *st, struct gl_linked_shader *shader,
> unsigned shader_type)
> {
> unsigned i;
> diff --git a/src/mesa/state_tracker/st_glsl_to_nir.cpp b/src/mesa/state_tracker/st_glsl_to_nir.cpp
> index a880564..52470a0 100644
> --- a/src/mesa/state_tracker/st_glsl_to_nir.cpp
> +++ b/src/mesa/state_tracker/st_glsl_to_nir.cpp
> @@ -347,7 +347,7 @@ st_finalize_nir(struct st_context *st, struct gl_program *prog, nir_shader *nir)
> struct gl_program *
> st_nir_get_mesa_program(struct gl_context *ctx,
> struct gl_shader_program *shader_program,
> - struct gl_shader *shader)
> + struct gl_linked_shader *shader)
> {
> struct gl_program *prog;
> GLenum target = _mesa_shader_stage_to_program(shader->Stage);
> diff --git a/src/mesa/state_tracker/st_glsl_to_tgsi.cpp b/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
> index 07ec91a..9315153 100644
> --- a/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
> +++ b/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
> @@ -376,7 +376,7 @@ public:
> struct gl_context *ctx;
> struct gl_program *prog;
> struct gl_shader_program *shader_program;
> - struct gl_shader *shader;
> + struct gl_linked_shader *shader;
> struct gl_shader_compiler_options *options;
>
> int next_temp;
> @@ -6452,7 +6452,7 @@ out:
> static struct gl_program *
> get_mesa_program_tgsi(struct gl_context *ctx,
> struct gl_shader_program *shader_program,
> - struct gl_shader *shader)
> + struct gl_linked_shader *shader)
> {
> glsl_to_tgsi_visitor* v;
> struct gl_program *prog;
> @@ -6663,7 +6663,7 @@ get_mesa_program_tgsi(struct gl_context *ctx,
> static struct gl_program *
> get_mesa_program(struct gl_context *ctx,
> struct gl_shader_program *shader_program,
> - struct gl_shader *shader)
> + struct gl_linked_shader *shader)
> {
> struct pipe_screen *pscreen = ctx->st->pipe->screen;
> unsigned ptarget = st_shader_stage_to_ptarget(shader->Stage);
> diff --git a/src/mesa/state_tracker/st_nir.h b/src/mesa/state_tracker/st_nir.h
> index 49ba573..19e2d2d 100644
> --- a/src/mesa/state_tracker/st_nir.h
> +++ b/src/mesa/state_tracker/st_nir.h
> @@ -43,7 +43,7 @@ void st_finalize_nir(struct st_context *st, struct gl_program *prog, nir_shader
> struct gl_program *
> st_nir_get_mesa_program(struct gl_context *ctx,
> struct gl_shader_program *shader_program,
> - struct gl_shader *shader);
> + struct gl_linked_shader *shader);
>
> #ifdef __cplusplus
> }
> diff --git a/src/mesa/state_tracker/st_program.c b/src/mesa/state_tracker/st_program.c
> index f2b5537..57b0935 100644
> --- a/src/mesa/state_tracker/st_program.c
> +++ b/src/mesa/state_tracker/st_program.c
> @@ -1756,10 +1756,6 @@ destroy_shader_program_variants_cb(GLuint key, void *data, void *userData)
> struct gl_shader_program *shProg = (struct gl_shader_program *) data;
> GLuint i;
>
> - for (i = 0; i < shProg->NumShaders; i++) {
> - destroy_program_variants(st, shProg->Shaders[i]->Program);
> - }
> -
> for (i = 0; i < ARRAY_SIZE(shProg->_LinkedShaders); i++) {
> if (shProg->_LinkedShaders[i])
> destroy_program_variants(st, shProg->_LinkedShaders[i]->Program);
> @@ -1772,9 +1768,6 @@ destroy_shader_program_variants_cb(GLuint key, void *data, void *userData)
> case GL_TESS_CONTROL_SHADER:
> case GL_TESS_EVALUATION_SHADER:
> case GL_COMPUTE_SHADER:
> - {
> - destroy_program_variants(st, shader->Program);
> - }
> break;
> default:
> assert(0);
More information about the mesa-dev
mailing list