On Wed, 30 Sep 2015 21:18:43 +0200
gregory hainaut <gregory.hain...@gmail.com> wrote:
> On Fri, 25 Sep 2015 16:40:31 -0700
> Ian Romanick <i...@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.
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
