[Mesa-dev] [PATCH 04/59] glsl/standalone: Enable par-linking

[Ian Romanick]

On 10/26/2016 02:25 AM, Iago Toral wrote:
> On Tue, 2016-10-25 at 17:59 -0700, Ian Romanick wrote:
>> From: Ian Romanick <ian.d.romanick at intel.com>
>>
>> If the user did not request full linking, link the shader with the
>> built-in functions, inline them, and eliminate them.  Previous to
>> this
>> you'd see all these calls to "dot" and "max" in the output.  This
>> prevented a lot of expected optimizations and cluttered the output.
>> This gives it some chance of being useful.
>>
>> v2: Rebase on top of Ken's "built-ins now" work.
>>
>> v3: Don't do_common_optimizations if par-linking fails.  Update
>> expected
>> output of warnings tests to prevent 'make check' regressions.
>>
>> v4: Optimize harder.  Most important, do function
>> inlining.  Otherwise
>> it's quite impractical for one function in a file to call another
>> function in the same file.
>>
>> Signed-off-by: Ian Romanick <ian.d.romanick at intel.com>
>> ---
>>  src/compiler/glsl/standalone.cpp                   | 43
>> +++++++++++++++++++++-
>>  ...-out-function-parameter-shaderout.vert.expected |  2 +
>>  ...nout-function-parameter-shaderout.vert.expected |  2 +
>>  .../030-array-as-function-parameter.vert.expected  |  2 +
>>  4 files changed, 47 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/compiler/glsl/standalone.cpp
>> b/src/compiler/glsl/standalone.cpp
>> index 6a4432f..ecf162e 100644
>> --- a/src/compiler/glsl/standalone.cpp
>> +++ b/src/compiler/glsl/standalone.cpp
>> @@ -38,6 +38,8 @@
>>  #include "standalone.h"
>>  #include "util/string_to_uint_map.h"
>>  #include "util/set.h"
>> +#include "linker.h"
>> +#include "glsl_parser_extras.h"
>>  
>>  class add_neg_to_sub_visitor : public ir_hierarchical_visitor {
>>  public:
>> @@ -508,10 +510,47 @@ standalone_compile_shader(const struct
>> standalone_options *_options,
>>        }
>>     }
>>  
>> -   if ((status == EXIT_SUCCESS) && options->do_link)  {
>> +   if (status == EXIT_SUCCESS) {
>>        _mesa_clear_shader_program_data(whole_program);
>>  
>> -      link_shaders(ctx, whole_program);
>> +      if (options->do_link)  {
>> +         link_shaders(ctx, whole_program);
>> +      } else {
>> +         struct gl_shader *const shader = whole_program->Shaders[0];
>>
> 
> It seems like you only need 'shader' to know to access 'shader->Stage'
> in the code below, so maybe just copy the stage here?
> 
>> +         whole_program->LinkStatus = GL_TRUE;
>> +         whole_program->_LinkedShaders[shader->Stage] =
>> +            link_intrastage_shaders(whole_program /* mem_ctx */,
>> +                                    ctx,
>> +                                    whole_program,
>> +                                    whole_program->Shaders,
>> +                                    1,
>> +                                    true);
>> +
>> +         /* Par-linking can fail, for example, if there are
>> undefined external
>> +          * references.
>> +          */
>> +         if (whole_program->_LinkedShaders[shader->Stage] != NULL) {
> 
> The expectation in this case would be that
> whole_program->LinkStatus is also GL_TRUE. Maybe worth adding an assert
> here to catch inconsistent error handling bugs in the linker?

I have applied both suggestions locally.  Thanks!

> Either way, this patch is:
> 
> Reviewed-by: Iago Toral Quiroga <itoral at igalia.com>
> 
>> +            struct gl_shader_compiler_options *const
>> compiler_options =
>> +               &ctx->Const.ShaderCompilerOptions[shader->Stage];
>> +
>> +            exec_list *const ir =
>> +               whole_program->_LinkedShaders[shader->Stage]->ir;
>> +
>> +            bool progress;
>> +            do {
>> +               progress = do_function_inlining(ir);
>> +
>> +               progress = do_common_optimization(ir,
>> +                                                 false,
>> +                                                 false,
>> +                                                 compiler_options,
>> +                                                 true)
>> +                  && progress;
>> +            } while(progress);
>> +         }
>> +      }
>> +
>>        status = (whole_program->LinkStatus) ? EXIT_SUCCESS :
>> EXIT_FAILURE;
>>  
>>        if (strlen(whole_program->InfoLog) > 0) {
>> diff --git a/src/compiler/glsl/tests/warnings/026-out-function-
>> parameter-shaderout.vert.expected
>> b/src/compiler/glsl/tests/warnings/026-out-function-parameter-
>> shaderout.vert.expected
>> index e69de29..60d3a8a 100644
>> --- a/src/compiler/glsl/tests/warnings/026-out-function-parameter-
>> shaderout.vert.expected
>> +++ b/src/compiler/glsl/tests/warnings/026-out-function-parameter-
>> shaderout.vert.expected
>> @@ -0,0 +1,2 @@
>> +
>> +error: unresolved reference to function `fooFunction'
>> diff --git a/src/compiler/glsl/tests/warnings/027-inout-function-
>> parameter-shaderout.vert.expected
>> b/src/compiler/glsl/tests/warnings/027-inout-function-parameter-
>> shaderout.vert.expected
>> index 1724975..651818d 100644
>> --- a/src/compiler/glsl/tests/warnings/027-inout-function-parameter-
>> shaderout.vert.expected
>> +++ b/src/compiler/glsl/tests/warnings/027-inout-function-parameter-
>> shaderout.vert.expected
>> @@ -1 +1,3 @@
>>  0:11(14): warning: `willBeDefined' used uninitialized
>> +
>> +error: unresolved reference to function `fooFunction'
>> diff --git a/src/compiler/glsl/tests/warnings/030-array-as-function-
>> parameter.vert.expected b/src/compiler/glsl/tests/warnings/030-array-
>> as-function-parameter.vert.expected
>> index 21cb2c5..b1355d3 100644
>> --- a/src/compiler/glsl/tests/warnings/030-array-as-function-
>> parameter.vert.expected
>> +++ b/src/compiler/glsl/tests/warnings/030-array-as-function-
>> parameter.vert.expected
>> @@ -5,3 +5,5 @@
>>  0:14(20): warning: `undefinedIndex' used uninitialized
>>  0:14(51): warning: `undefinedIndex' used uninitialized
>>  0:14(82): warning: `undefinedIndex' used uninitialized
>> +
>> +error: unresolved reference to function `foo'
>