[Mesa-dev] Geometry shader update, and a course correction

[Ian Romanick]

On 07/19/2013 11:48 AM, Paul Berry wrote:
> (TL;DR: geometry shaders are humming along, but because of a hitch I've
> run into, I'm going to change gears and implement GLSL 1.50-style
> geometry shaders first rather than ARB_geometry_shader4 functionality.
> This means some piglit tests will need to be rewritten).
>
> Hello all--
>
> As some of you may have noticed, I've spent the last several weeks
> writing a number of geometry shader tests and fixinging my "gs" branch
> (on https://github.com/stereotype441/mesa.git).  I'm happy to report
> that it's working better than ever: I now have the following features
> working in geometry shaders on Intel drivers:
>
> - Texturing.
> - Built-in uniform variables.
> - Interactions with ARB_shader_texture_lod,
> ARB_shading_language_packing, ARB_texture_cube_map_array,
> ARB_texture_multisample, ARB_texture_rectangle,
> ARB_uniform_buffer_object, and EXT_texture_array.
> - Mixing user-defined geometry shaders with fixed-function fragment shaders.
> - gl_PointSizeIn, gl_PrimitiveIDIn, gl_PrimitiveID, gl_ClipVertex,
> gl_ClipDistanceIn, and gl_ClipDistance (including several important
> corner cases of gl_ClipDistance that were previously broken, such as
> bulk assignment).
> - Color clamping.
>
> I've done all of this work using the ARB_geometry_shader4 version of
> geometry shaders; my intention was to get that working first, then
> introduce additional code to support GLSL 1.50-style geometry shaders.
> This was partly motivated by the fact that Bryan Cain's initial work on
> geometry shaders was done using ARB_geometry_shader4, and partly
> motivated by the fact that it's our standard development practice in
> Mesa to develop features as extensions first, and then only support a
> core feature once all of the extensions it includes have been finished.
> However, I've now run into hitch that's making me want to change course
> and implement GLSL-1.50 style geometry shaders first.  It's a bit of a
> long story so buckle up:
>
> In GLSL 1.50, the way you specify the input primitive type for a
> geometry shader is using a layout qualifier in the shader source code,
> so it is known at compile time.  Whereas in ARB_geometry_shader4, the
> way you specify the input primitive type is using
> glProgramParameteriARB() prior to linking.  This means that the input
> primitive type isn't known at compile time.
>
> That's a problem because ARB_geometry_shader4 provides for a read-only
> built-in constant called gl_VerticesIn, which is the number of vertices
> in each geometry shader input primitive (e.g. if the input primitive
> type is "triangles", gl_VerticesIn is 3).  Mesa does not currently
> support constants whose value is determined at link time, since constant
> folding, determination of explicit array sizes, and a lot of
> constant-related error checking happen at compile time.  To work around
> this, the "gs" branch currently treats gl_VerticesIn as a varying input
> during compilation, and then at link time replaces any references to it
> with the appropriate constant value.

Oh dear.  That's... awful.

There is another possible work-around for this problem, but I don't know 
that it's any better.  gl_VerticesIn has a limited set of possible 
value: 1, 2, 3, 4, and 6.  We could ast-to-hir for each of those values. 
  We'd then pick the "right" one link time.  There would also be some 
weirdness with cases were only a subset compile (e.g., because the 
shader does something like 'float foo[gl_VerticesIn-2];' which would 
fail to compile when gl_VerticesIn <= 2).

Before committing to this route we should:

1. Survey known apps (e.g., on Steam) to see if there are any apps that 
try to use OpenGL 3.0 + GL_ARB_geometry_shader4.

2. Determine whether there is any hardware supported by Mesa that will 
never get OpenGL 3.0 (due to, say, one missing feature), but might get 
GLSL 1.30 + GL_ARB_geometry_shader4.

Just going for 3.2 and punting on the extension for now seems like the 
right way to go, but I also don't want to paint ourselves into a "we 
have to rearchitect the world" kind of corner. :(

> That in turn creates a problem because it prevents declarations like
> this from working:
>
> varying in float foo[gl_VerticesIn];
>
> since array sizes must be integral-constant expressions.  To work around
> this, the "gs" branch currently modifies the parser so that the above
> declaration is treated as equivalent to an unsized array, i.e.:
>
> varying in float foo[];
>
> Similar changes are made for 2D inputs such as gl_TexCoordIn.
>
> That in turn is a problem because it prevents code like this from working:
>
> varying in float foo[gl_VerticesIn];
> varying out float bar;
> void main()
> {
>    for (int i = 0; i < gl_VerticesIn; i++) {
>      bar = foo[i]; /* problem */
>      EmitVertex();
>    }
> }
>
> since the GLSL spec stipulates that it is only legal to index into an
> unsized array using an integral constant expression.  To work around
> that, this stipulation is suspended when ARB_geometry_shader4 is in use.
>
> At this point, we have three layers of workarounds being applied because
> of one crazy feature in ARB_geometry_shader4 that doesn't exist in GLSL
> 1.50 style geometry shaders.  Some of these workarounds have unintended
> consequences that aren't easy to fix.  For example, the following shader
> is accepted, even though it should be rejected:
>
> varing in float foo[];
> varying out float bar;
> void main()
> {
>    for (int i = 0; i < gl_VerticesIn; i++) {
>      bar = foo[i]; /* illegal: i must be a constant because foo is
> unsized */
>      EmitVertex();
>    }
> }
>
> Conversely, code like this fails, even though it is allowed by
> ARB_geometry_shader4:
>
> varying in float foo[gl_VerticesIn + 1 - 1];
>
> And so does this:
>
> int main()
> {
>    float tmp[gl_VerticesIn];
> }
>
> Note that none of these problems applies to GLSL 1.50 style geometry
> shaders.
>
> I'm not comfortable with this situation.  I would far prefer to remove
> the workarounds, and instead have Mesa postpone compilation of
> ARB_geometry_shader4-style geometry shaders until link time, when the
> value of gl_VerticesIn is known.  That would make the code cleaner, and
> eliminate all of the corner cases that are currently broken, at the
> expense of making certain compile errors reported at link time rather
> than compile time (competing implementations, such as Nvidia's
> proprietary driver, have this behaviour already).
>
> However, that's going to be a fair amount of effort, and it's effort
> that doesn't really push us towards GL 3.2 (which is the primary reason
> I'm interested in working on geometry shaders to begin with).  So
> instead, I'm going to shift gears and modify my "gs" branch so that it
> implements GLSL 1.50 style geometry shaders rather than
> ARB_geometry_shader4 style geometry shaders.  Once GLSL 1.50 is working
> and committed upstream, I'll consider working on ARB_geometry_shader4
> again, if there is interest.
>
> An additional advantage of shifting gears to GLSL 1.50 style geometry
> shaders is that we no longer have to worry about interactions between
> geometry shaders and compatibility-only features (since Mesa currently
> only implements the core profile of GL 3.1 and above).  It also means
> that we don't have to worry about subtle bugs creeping in because of
> ARB_geometry_shader4's use of 2D arrays (a lot of code in src/glsl
> assumes that arrays are always 1D, and I haven't yet convinced myself
> that this assumption has been removed when necessary for geometry
> shaders).  Finally, it's possible that a high enough percentage of the
> users of geometry shaders will do so through via GLSL 1.50, so it may
> never be necessary to implement ARB_geometry_shader4 at all.
>
> One impact of this course change is that many of the existing piglit
> tests which validate geometry shaders currently use
> ARB_geometry_shader4.  We'll need to write counterparts to these tests
> that use GLSL 1.50.  Intel has a few interns working for us this
> summer--I think I may try to talk them into doing that work :)
>
> I've taken a snapshot of the current state of my "gs" branch and called
> it "gs-arb", so if we later find out this was a bad idea, we can switch
> back to plan A without much penalty.
>
> I believe this is the right way to go, but I'm open to any comments
> anyone has on this plan.  Thanks in advance!
>
> Paul
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev