Kenneth Graunke <kenn...@whitecape.org> writes: > 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
>> @@ -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.
Fixed.
>> 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.
This hunk was a leftover from when I was trying to name the new compiler
function compile_shader() (like the linker one is named link_shaders()).
>> @@ -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...
It was to make the code movement from ir_to_mesa require less change.
> 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.
Huh, I didn't think this was going to be hard to review. This function
in ir_to_mesa.cpp had no business being in ir_to_mesa.cpp because it's
not about ir_to_mesa, so I moved its contents to the two appropriate
locations. I guess I'll split the patch up into a step per appropriate
location.
pgpCjk0ZCunDI.pgp
Description: PGP signature
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
