[Mesa-dev] [PATCH 0/3][RFC] Improve GLSL support of GL_ARB_separate_shader_objects

[Ian Romanick]

On 09/20/2015 01:15 PM, Gregory Hainaut wrote:
> Current GLSL badly optimizes the code making it incompatible with the
> GL_ARB_separate_shader_objects extension.
> 
> Typical example of the current behavior:
> 
> VS:
> out SHADER
> {
>     vec4 var_zero; // location will always be 0
>     vec2 var_one; // location will always be 1
> } VSout;
> 
> FS:
> in SHADER
> {
>     vec4 var_zero;
>     vec2 var_one; // ERROR: location would be wrongly set to 0 if var_zero is inactive
> } PSin;
> 
> In short, SSO allow to match by name but you can't make any hypothesis on the
> previous/next stage. Therefore you must consider all inputs as actives.

Welcome back. :)

I think I understand what is happening, but I think the approach is not
quite right.  My understanding, with the aid of the spec quote in patch
3, is that any interstage I/O (i.e., not vertex inputs and not fragment
outputs) that are explicitly declared in a stage, even if unused in that
stage, cannot be eliminated.  This is roughly like layout(shared) on UBOs.

This set of changes only prevents unused interstage inputs from being
eliminated.  It does nothing for vertex, geometry, or tessellation
outputs.  It also prevents the elimination of user-declared inputs that
could previously be eliminated.  It seems that if the VS and FS
mentioned above were linked together, var_zero would (incorrectly) not
be eliminated.  Right?

I think patches 1 and 2 should just be dropped.  A new patch 1 should
add a method that determines whether or not a ir_var_shader_in or
ir_var_shader_out is interstage.  I thought such a function existed, but
I could not find it.  The replacement for patch 3 would change the logic
to "if it's interstage I/O, and there isn't an explicit location, and
this stage isn't linked with a following stage, do not eliminate the
variable."

The "this stage isn't linked with a following stage" part is important
because we still want to eliminate vertex shader outputs in separable
shaders if the vertex shader is linked directly with (only) a geometry
shader, for example.

> Commit decription:
> patch 1/ Tighten up some linker error checks to avoid dependency with the
>          deadcode optimization phase. Phase that will be partially removed in
>          patch 3.
> 
> patch 2/ Add a new variable flags in the AST to distinguish attribute from
>          varying. It is still legal to removed unused attribute.
> 
> patch 3/ Disable the optimization of varying inputs that don't have an explicit
>          location. The idea was to keep the optimization for all builtins
>          varyings. Besides it won't trigger the bug if explicit location are
>          used for all variables. I'm not sure what it is the best solutions.
> 
> 
> Piglit test done on older Mesa/Nouveau (8276ba260) due to libdrm newer requirement.
> 
> [30454/30454] crash: 29, fail: 221, pass: 20749, skip: 9451, warn: 4 -
> 
> The fix was validated on the PCSX2 application. I will try to create a
> piglit tests of the above situation.
> 
> 
> Gregory Hainaut (3):
>   glsl/link: don't consider unused variable as read variable.
>   glsl/ast2hir: add a new flag attribute_input for VS input
>   glsl/opt: disable optimization of varying input
> 
>  src/glsl/ast_to_hir.cpp    |  4 ++++
>  src/glsl/ir.h              |  9 +++++++++
>  src/glsl/link_varyings.cpp |  4 ++--
>  src/glsl/opt_dead_code.cpp | 16 ++++++++++++++++
>  4 files changed, 31 insertions(+), 2 deletions(-)
>