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

[gregory hainaut]

On Fri, 25 Sep 2015 16:40:31 -0700
Ian Romanick <idr at freedesktop.org> wrote:

> 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. :)

Oh, thanks you :)
 
> 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.

Yes
 
> 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?

Yes, you're right. I extended my current piglit test to check 
both input and output wrong that are wrongly eliminated. I also made a small
test to check interstage optimization.

> 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.
> 
So far, I manage to annotate the ir with an interstage flag. However some issues
are remaining with explicit location.

Here the bad FS program:

layout(location = 0) in vec4 blue;
in vec4 green;
in vec4 red;

void main()
{
    out_color = vec4(green.xyz, 1);
}

// IR for the FS
(declare (location=26 shader_in ) vec4 green)
(declare (location=27 shader_in ) vec4 red)
(declare (location=4 shader_out ) vec4 out_color)

// IR for the VS (same interface but declared as out)
(declare (location=0 shader_out ) vec4 gl_Position)
(declare (temporary ) vec4 gl_Position)
(declare (location=26 shader_out ) vec4 blue)
(declare (temporary ) vec4 blue)
(declare (location=26 shader_out flat) vec4 green)
(declare (temporary ) vec4 green)
(declare (location=27 shader_out flat) vec4 red)
(declare (temporary ) vec4 red)
(declare (location=17 shader_in ) vec4 piglit_vertex)

As you can see there are 2 problems.
1/ location in the VS overlap, 26 appears 2 times.
I'm not sure that Mesa support it actually (or if it isn't allowed).

2/ FS doesn't reserve slot 26 (VARYING_SLOT_VAR0) for blue and start directly to the slot 26
Technically I could just disable the optimization of those variables too. But sadly
I hit issue number 1 for FS too.

Any idea of where it could be wrong?

Thanks.