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@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev > _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev