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

[gregory hainaut]

On Wed, 30 Sep 2015 21:18:43 +0200
gregory hainaut <gregory.hainaut at gmail.com> wrote:

> 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.
> 
Hum, just further reading of the code, I found a comment in
link_invalidate_variable_locations that code must be updated for
GL_ARB_separate_shader_objects. In the above test, the explicit variable
has is_unmatched_generic_layout = 0 so varying_matches::record doesn't
record it. Not sure if the comment is still valid.

Otherwise, it seems that varying_matches::assign_locations doesn't take into account 
already reserved locations.