[Mesa-dev] [PATCH 5/7] glsl/mesa: split gl_shader in two
Timothy Arceri
timothy.arceri at collabora.com
Wed Jun 29 12:54:28 UTC 2016
On Wed, 2016-06-29 at 14:36 +0200, Iago Toral wrote:
> 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?
I didn't but I can. However this is only used to print gl shader
objects so Program should never be set as it only gets set to linked
shaders.
I was thinking yesterday I should really have split this and the change
below into separate patches, I can do that if you like?
>
> > 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.
This is only used to print linked shaders which always have a name of 0
so this is kind of pointless.
>
> 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);
>
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
More information about the mesa-dev
mailing list