[Mesa-dev] [PATCH 6/6] mesa: Move the common _mesa_glsl_compile_shader() code to glsl/.
Kenneth Graunke
kenneth at whitecape.org
Tue Jun 18 04:05:22 PDT 2013
On 06/17/2013 04:10 PM, Eric Anholt wrote:
> ... and move the mesa-core-specific code into Mesa core. This code had no
> relation to ir_to_mesa.cpp, since it was also used by intel and
> state_tracker, and most of it was duplicated with the standalone compiler
> (which has periodically drifted from the Mesa copy).
> ---
> src/glsl/glsl_parser_extras.cpp | 83 ++++++++++++++++++++++++++++++++++++
> src/glsl/glsl_parser_extras.h | 2 +
> src/glsl/main.cpp | 59 +-------------------------
> src/glsl/program.h | 16 ++++++-
> src/mesa/main/shaderapi.c | 57 ++++++++++++++++++++-----
> src/mesa/program/ir_to_mesa.cpp | 94 -----------------------------------------
> src/mesa/program/ir_to_mesa.h | 1 -
> 7 files changed, 146 insertions(+), 166 deletions(-)
>
> diff --git a/src/glsl/glsl_parser_extras.cpp b/src/glsl/glsl_parser_extras.cpp
> index 7b827ba..f142d73 100644
> --- a/src/glsl/glsl_parser_extras.cpp
> +++ b/src/glsl/glsl_parser_extras.cpp
> @@ -28,6 +28,7 @@
> extern "C" {
> #include "main/core.h" /* for struct gl_context */
> #include "main/context.h"
> +#include "main/shaderobj.h"
> }
>
> #include "ralloc.h"
> @@ -1237,6 +1238,88 @@ ast_struct_specifier::ast_struct_specifier(const char *identifier,
> this->declarations.push_degenerate_list_at_head(&declarator_list->link);
> }
>
> +extern "C" {
> +
> +void
> +_mesa_glsl_compile_shader(struct gl_context *ctx, struct gl_shader *shader,
> + bool dump_ast, bool dump_hir)
> +{
> + struct _mesa_glsl_parse_state *state =
> + new(shader) _mesa_glsl_parse_state(ctx, shader->Type, shader);
> + const char *source = shader->Source;
> +
> + state->error = glcpp_preprocess(state, &source, &state->info_log,
> + &ctx->Extensions, ctx);
> +
> + if (!state->error) {
> + _mesa_glsl_lexer_ctor(state, source);
> + _mesa_glsl_parse(state);
> + _mesa_glsl_lexer_dtor(state);
> + }
> +
> + if (dump_ast) {
> + foreach_list_const(n, &state->translation_unit) {
> + ast_node *ast = exec_node_data(ast_node, n, link);
> + ast->print();
> + }
> + printf("\n\n");
> + }
> +
> + ralloc_free(shader->ir);
> + shader->ir = new(shader) exec_list;
> + if (!state->error && !state->translation_unit.is_empty())
> + _mesa_ast_to_hir(shader->ir, state);
> +
> + if (!state->error) {
> + validate_ir_tree(shader->ir);
> +
> + /* Print out the unoptimized IR. */
> + if (dump_hir) {
> + _mesa_print_ir(shader->ir, state);
> + }
> + }
> +
> +
> + if (!state->error && !shader->ir->is_empty()) {
> + struct gl_shader_compiler_options *options =
> + &ctx->ShaderCompilerOptions[_mesa_shader_type_to_index(shader->Type)];
> +
> + /* Do some optimization at compile time to reduce shader IR size
> + * and reduce later work if the same shader is linked multiple times
> + */
> + while (do_common_optimization(shader->ir, false, false, 32, options))
> + ;
> +
> + validate_ir_tree(shader->ir);
> + }
> +
> + if (shader->InfoLog)
> + ralloc_free(shader->InfoLog);
> +
> + shader->symbols = state->symbols;
> + shader->CompileStatus = !state->error;
> + shader->InfoLog = state->info_log;
> + shader->Version = state->language_version;
> + shader->InfoLog = state->info_log;
> + shader->IsES = state->es_shader;
> +
> + memcpy(shader->builtins_to_link, state->builtins_to_link,
> + sizeof(shader->builtins_to_link[0]) * state->num_builtins_to_link);
> + shader->num_builtins_to_link = state->num_builtins_to_link;
> +
> + if (shader->UniformBlocks)
> + ralloc_free(shader->UniformBlocks);
> + shader->NumUniformBlocks = state->num_uniform_blocks;
> + shader->UniformBlocks = state->uniform_blocks;
> + ralloc_steal(shader, shader->UniformBlocks);
> +
> + /* Retain any live IR, but trash the rest. */
> + reparent_ir(shader->ir, shader->ir);
> +
> + ralloc_free(state);
> +}
> +
> +} /* extern "C" */
> /**
> * Do the set of common optimizations passes
> *
The above change looks good. It makes a lot of sense for this to be in
the compiler.
There are tabs, if you care. Not a big deal either way.
> diff --git a/src/glsl/glsl_parser_extras.h b/src/glsl/glsl_parser_extras.h
> index 7f478df..cf296e9 100644
> --- a/src/glsl/glsl_parser_extras.h
> +++ b/src/glsl/glsl_parser_extras.h
> @@ -342,6 +342,8 @@ extern int _mesa_glsl_lex(union YYSTYPE *yylval, YYLTYPE *yylloc,
>
> extern int _mesa_glsl_parse(struct _mesa_glsl_parse_state *);
>
> +extern void compile_shader();
> +
> /**
> * Process elements of the #extension directive
> *
I am bewildered by this hunk. There are compile_shader() functions in
both glsl/main.cpp and mesa/main/shaderapi.c, but both of those are
static, and your patch doesn't change that. I can't find /anything/
that this would refer to.
It almost looks like you split things differently in an earlier version
of the patch and this got left in.
> diff --git a/src/glsl/main.cpp b/src/glsl/main.cpp
> index 5713ee5..60bc628 100644
> --- a/src/glsl/main.cpp
> +++ b/src/glsl/main.cpp
> @@ -143,70 +143,13 @@ compile_shader(struct gl_context *ctx, struct gl_shader *shader)
> struct _mesa_glsl_parse_state *state =
> new(shader) _mesa_glsl_parse_state(ctx, shader->Type, shader);
>
> - const char *source = shader->Source;
> - state->error = glcpp_preprocess(state, &source, &state->info_log,
> - state->extensions, ctx) != 0;
> -
> - if (!state->error) {
> - _mesa_glsl_lexer_ctor(state, source);
> - _mesa_glsl_parse(state);
> - _mesa_glsl_lexer_dtor(state);
> - }
> -
> - if (dump_ast) {
> - foreach_list_const(n, &state->translation_unit) {
> - ast_node *ast = exec_node_data(ast_node, n, link);
> - ast->print();
> - }
> - printf("\n\n");
> - }
> -
> - shader->ir = new(shader) exec_list;
> - if (!state->error && !state->translation_unit.is_empty())
> - _mesa_ast_to_hir(shader->ir, state);
> -
> - /* Print out the unoptimized IR. */
> - if (!state->error && dump_hir) {
> - validate_ir_tree(shader->ir);
> - _mesa_print_ir(shader->ir, state);
> - }
> -
> - /* Optimization passes */
> - if (!state->error && !shader->ir->is_empty()) {
> - const struct gl_shader_compiler_options *opts =
> - &ctx->ShaderCompilerOptions[_mesa_shader_type_to_index(shader->Type)];
> - bool progress;
> - do {
> - progress = do_common_optimization(shader->ir, false, false, 32, opts);
> - } while (progress);
> -
> - validate_ir_tree(shader->ir);
> - }
> -
> + _mesa_glsl_compile_shader(ctx, shader, dump_ast, dump_hir);
>
> /* Print out the resulting IR */
> if (!state->error && dump_lir) {
> _mesa_print_ir(shader->ir, state);
> }
>
> - shader->symbols = state->symbols;
> - shader->CompileStatus = !state->error;
> - shader->Version = state->language_version;
> - shader->IsES = state->es_shader;
> - memcpy(shader->builtins_to_link, state->builtins_to_link,
> - sizeof(shader->builtins_to_link[0]) * state->num_builtins_to_link);
> - shader->num_builtins_to_link = state->num_builtins_to_link;
> -
> - if (shader->InfoLog)
> - ralloc_free(shader->InfoLog);
> -
> - shader->InfoLog = state->info_log;
> -
> - /* Retain any live IR, but trash the rest. */
> - reparent_ir(shader->ir, shader);
> -
> - ralloc_free(state);
> -
> return;
> }
This looks reasonable.
> diff --git a/src/glsl/program.h b/src/glsl/program.h
> index 6a76d4d..f15113a 100644
> --- a/src/glsl/program.h
> +++ b/src/glsl/program.h
> @@ -24,15 +24,27 @@
>
> #include "main/core.h"
>
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +extern void
> +_mesa_glsl_compile_shader(struct gl_context *ctx, struct gl_shader *shader,
> + bool dump_ast, bool dump_hir);
> +
> +#ifdef __cplusplus
> +} /* extern "C" */
> +#endif
> +
> extern void
> link_shaders(struct gl_context *ctx, struct gl_shader_program *prog);
>
> extern void
> -linker_error(gl_shader_program *prog, const char *fmt, ...)
> +linker_error(struct gl_shader_program *prog, const char *fmt, ...)
> PRINTFLIKE(2, 3);
>
> extern void
> -linker_warning(gl_shader_program *prog, const char *fmt, ...)
> +linker_warning(struct gl_shader_program *prog, const char *fmt, ...)
> PRINTFLIKE(2, 3);
>
> extern long
> diff --git a/src/mesa/main/shaderapi.c b/src/mesa/main/shaderapi.c
> index 88518ca..4f9e785 100644
> --- a/src/mesa/main/shaderapi.c
> +++ b/src/mesa/main/shaderapi.c
> @@ -48,11 +48,14 @@
> #include "main/transformfeedback.h"
> #include "main/uniforms.h"
> #include "program/program.h"
> +#include "program/prog_print.h"
> #include "program/prog_parameter.h"
> #include "ralloc.h"
> #include <stdbool.h>
> #include "../glsl/glsl_parser_extras.h"
> +#include "../glsl/ir.h"
> #include "../glsl/ir_uniform.h"
> +#include "../glsl/program.h"
>
> /** Define this to enable shader substitution (see below) */
> #define SHADER_SUBST 0
> @@ -731,27 +734,59 @@ shader_source(struct gl_context *ctx, GLuint shader, const GLchar *source)
> static void
> compile_shader(struct gl_context *ctx, GLuint shaderObj)
> {
> - struct gl_shader *sh;
> + struct gl_shader *shader;
I don't see the point in renaming "sh" to "shader." It's not a horrible
idea, but with the code motion, converting code to be both C and C++
compatible, and include changes, there's enough going on without extra
accidental complexity...
> struct gl_shader_compiler_options *options;
>
> - sh = _mesa_lookup_shader_err(ctx, shaderObj, "glCompileShader");
> - if (!sh)
> + shader = _mesa_lookup_shader_err(ctx, shaderObj, "glCompileShader");
> + if (!shader)
> return;
>
> - options = &ctx->ShaderCompilerOptions[_mesa_shader_type_to_index(sh->Type)];
> + options = &ctx->ShaderCompilerOptions[_mesa_shader_type_to_index(shader->Type)];
>
> /* set default pragma state for shader */
> - sh->Pragmas = options->DefaultPragmas;
> + shader->Pragmas = options->DefaultPragmas;
>
> - /* this call will set the sh->CompileStatus field to indicate if
> - * compilation was successful.
> - */
> - _mesa_glsl_compile_shader(ctx, sh);
> + if (!shader->Source) {
> + /* If the user called glCompileShader without first calling
> + * glShaderSource, we should fail to compile, but not raise a GL_ERROR.
> + */
> + shader->CompileStatus = GL_FALSE;
> + } else {
> + if (ctx->Shader.Flags & GLSL_DUMP) {
> + printf("GLSL source for %s shader %d:\n",
> + _mesa_glsl_shader_target_name(shader->Type), shader->Name);
> + printf("%s\n", shader->Source);
> + }
> +
> + /* this call will set the shader->CompileStatus field to indicate if
> + * compilation was successful.
> + */
> + _mesa_glsl_compile_shader(ctx, shader, false, false);
> +
> + if (ctx->Shader.Flags & GLSL_LOG) {
> + _mesa_write_shader_to_file(shader);
> + }
> +
> + if (ctx->Shader.Flags & GLSL_DUMP) {
> + if (shader->CompileStatus) {
> + printf("GLSL IR for shader %d:\n", shader->Name);
> + _mesa_print_ir(shader->ir, NULL);
> + printf("\n\n");
> + } else {
> + printf("GLSL shader %d failed to compile.\n", shader->Name);
> + }
> + if (shader->InfoLog && shader->InfoLog[0] != 0) {
> + printf("GLSL shader %d info log:\n", shader->Name);
> + printf("%s\n", shader->InfoLog);
> + }
> + }
> +
> + }
>
> - if (sh->CompileStatus == GL_FALSE &&
> + if (shader->CompileStatus == GL_FALSE &&
> (ctx->Shader.Flags & GLSL_REPORT_ERRORS)) {
> _mesa_debug(ctx, "Error compiling shader %u:\n%s\n",
> - sh->Name, sh->InfoLog);
> + shader->Name, shader->InfoLog);
> }
> }
Okay, now I'm really confused. I thought the point of this patch was to
unify two pieces of code, or share more code. It looks like you just
moved code from main.cpp into glsl_parser_extras.cpp, and *additionally*
moved code from ir_to_mesa.cpp into shaderapi.c, but in the same patch
*for some reason*.
Judging by the fact that Paul only gave this an Acked-by, I'm going to
wager he had trouble reviewing it as well. Could you please respin and
possibly split this up into patches that move one thing at a time with a
separate rationale for each?
Sorry for the trouble.
> diff --git a/src/mesa/program/ir_to_mesa.cpp b/src/mesa/program/ir_to_mesa.cpp
> index a915974..35a9b84 100644
> --- a/src/mesa/program/ir_to_mesa.cpp
> +++ b/src/mesa/program/ir_to_mesa.cpp
> @@ -3097,100 +3097,6 @@ _mesa_ir_link_shader(struct gl_context *ctx, struct gl_shader_program *prog)
> return prog->LinkStatus;
> }
>
> -
> -/**
> - * Compile a GLSL shader. Called via glCompileShader().
> - */
> -void
> -_mesa_glsl_compile_shader(struct gl_context *ctx, struct gl_shader *shader)
> -{
> - struct _mesa_glsl_parse_state *state =
> - new(shader) _mesa_glsl_parse_state(ctx, shader->Type, shader);
> -
> - const char *source = shader->Source;
> - /* Check if the user called glCompileShader without first calling
> - * glShaderSource. This should fail to compile, but not raise a GL_ERROR.
> - */
> - if (source == NULL) {
> - shader->CompileStatus = GL_FALSE;
> - return;
> - }
> -
> - state->error = glcpp_preprocess(state, &source, &state->info_log,
> - &ctx->Extensions, ctx);
> -
> - if (ctx->Shader.Flags & GLSL_DUMP) {
> - printf("GLSL source for %s shader %d:\n",
> - _mesa_glsl_shader_target_name(state->target), shader->Name);
> - printf("%s\n", shader->Source);
> - }
> -
> - if (!state->error) {
> - _mesa_glsl_lexer_ctor(state, source);
> - _mesa_glsl_parse(state);
> - _mesa_glsl_lexer_dtor(state);
> - }
> -
> - ralloc_free(shader->ir);
> - shader->ir = new(shader) exec_list;
> - if (!state->error && !state->translation_unit.is_empty())
> - _mesa_ast_to_hir(shader->ir, state);
> -
> - if (!state->error && !shader->ir->is_empty()) {
> - validate_ir_tree(shader->ir);
> - struct gl_shader_compiler_options *options =
> - &ctx->ShaderCompilerOptions[_mesa_shader_type_to_index(shader->Type)];
> -
> - /* Do some optimization at compile time to reduce shader IR size
> - * and reduce later work if the same shader is linked multiple times
> - */
> - while (do_common_optimization(shader->ir, false, false, 32, options))
> - ;
> -
> - validate_ir_tree(shader->ir);
> - }
> -
> - shader->symbols = state->symbols;
> -
> - shader->CompileStatus = !state->error;
> - shader->InfoLog = state->info_log;
> - shader->Version = state->language_version;
> - shader->IsES = state->es_shader;
> - memcpy(shader->builtins_to_link, state->builtins_to_link,
> - sizeof(shader->builtins_to_link[0]) * state->num_builtins_to_link);
> - shader->num_builtins_to_link = state->num_builtins_to_link;
> -
> - if (ctx->Shader.Flags & GLSL_LOG) {
> - _mesa_write_shader_to_file(shader);
> - }
> -
> - if (ctx->Shader.Flags & GLSL_DUMP) {
> - if (shader->CompileStatus) {
> - printf("GLSL IR for shader %d:\n", shader->Name);
> - _mesa_print_ir(shader->ir, NULL);
> - printf("\n\n");
> - } else {
> - printf("GLSL shader %d failed to compile.\n", shader->Name);
> - }
> - if (shader->InfoLog && shader->InfoLog[0] != 0) {
> - printf("GLSL shader %d info log:\n", shader->Name);
> - printf("%s\n", shader->InfoLog);
> - }
> - }
> -
> - if (shader->UniformBlocks)
> - ralloc_free(shader->UniformBlocks);
> - shader->NumUniformBlocks = state->num_uniform_blocks;
> - shader->UniformBlocks = state->uniform_blocks;
> - ralloc_steal(shader, shader->UniformBlocks);
> -
> - /* Retain any live IR, but trash the rest. */
> - reparent_ir(shader->ir, shader->ir);
> -
> - ralloc_free(state);
> -}
> -
> -
> /**
> * Link a GLSL shader program. Called via glLinkProgram().
> */
> diff --git a/src/mesa/program/ir_to_mesa.h b/src/mesa/program/ir_to_mesa.h
> index aa053d9..2488a45 100644
> --- a/src/mesa/program/ir_to_mesa.h
> +++ b/src/mesa/program/ir_to_mesa.h
> @@ -33,7 +33,6 @@ struct gl_context;
> struct gl_shader;
> struct gl_shader_program;
>
> -void _mesa_glsl_compile_shader(struct gl_context *ctx, struct gl_shader *sh);
> void _mesa_glsl_link_shader(struct gl_context *ctx, struct gl_shader_program *prog);
> GLboolean _mesa_ir_compile_shader(struct gl_context *ctx, struct gl_shader *shader);
> GLboolean _mesa_ir_link_shader(struct gl_context *ctx, struct gl_shader_program *prog);
>
More information about the mesa-dev
mailing list