[Mesa-dev] [PATCH 5/7] glsl/mesa: split gl_shader in two
Iago Toral
itoral at igalia.com
Wed Jun 29 13:01:58 UTC 2016
On Wed, 2016-06-29 at 22:54 +1000, Timothy Arceri wrote:
> 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.
Right, good point.
> I was thinking yesterday I should really have split this and the change
> below into separate patches, I can do that if you like?
Yeah, I think that would be better, thanks!
> >
> > > 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.
Aha, ok.
> >
> > 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