[Mesa-dev] [PATCH 6/6] mesa: Move the common _mesa_glsl_compile_shader() code to glsl/.

[Eric Anholt]

Kenneth Graunke <kenneth at 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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20130619/fe51ffab/attachment-0001.pgp>