[Mesa-dev] [PATCH RFC 3/3] glsl: Rework builtin_variables.cpp to reduce code duplication.

[Paul Berry]

On 11 July 2013 16:30, Ian Romanick <idr at freedesktop.org> wrote:

> On 07/11/2013 03:52 PM, Paul Berry wrote:
>
>> On 11 July 2013 14:09, Ian Romanick <idr at freedesktop.org
>> <mailto:idr at freedesktop.org>> wrote:
>>
>>     I like these changes a lot.  I have a few comments below.
>>
>>
>>     On 07/08/2013 10:40 AM, Paul Berry wrote:
>>
>>         Previously, we had a separate function for setting up the built-in
>>         variables for each combination of shader stage and GLSL version
>>         (e.g. generate_110_vs_variables to generate the built-in
>>         variables for
>>         GLSL 1.10 vertex shaders).  The functions called each other in
>>         ad-hoc
>>         ways, leading to unexpected inconsistencies (for example,
>>         generate_120_fs_variables was called for GLSL versions 1.20 and
>>         above,
>>         but generate_130_fs_variables was called only for GLSL version
>>         1.30).
>>         In addition, it led to a lot of code duplication, since many
>>         varyings
>>         had to be duplicated in both the FS and VS code paths.  With the
>>         advent of geometry shaders (and later, tessellation control and
>>         tessellation evaluation shaders), this code duplication was going
>> to
>>         get a lot worse.
>>
>>         So this patch reworks things so that instead of having a separate
>>         function for each shader type and GLSL version, we have a
>>         function for
>>         constants, one for uniforms, one for varyings, and one for the
>>         special
>>         variables that are specific to each shader type.
>>
>>         In addition, we use a class, builtin_variable_generator, to keep
>>         track
>>         of the instruction exec_list, the GLSL parse state, commonly-used
>>         types, and a few other variables, so that we don't have to pass
>> them
>>         around as function arguments.  This makes the code a lot more
>>         compact.
>>
>>         Where it was feasible to do so without introducing compilation
>>         errors,
>>         I've also gone ahead and introduced the variables needed for
>>         {ARB,EXT}_geometry_shader4 style geometry shaders.  This patch
>> takes
>>         care of everything except the GS variable gl_VerticesIn, the FS
>>         variable gl_PrimitiveID, and GLSL 1.50 style geometry shader
>> inputs
>>         (using the gl_in interface block).  Those remaining features will
>> be
>>         added later.
>>
>>         I've also made a slight nomenclature change: previously we used
>> the
>>         word "deprecated" to refer to variables which are marked in GLSL
>>         1.40
>>         as requiring the ARB_compatibility extension, and are marked in
>> GLSL
>>         1.50 onward as requiring the compatibilty profile.  This was
>>         misleading, since not all deprecated variables require the
>>         compatibility profile (for example gl_FragData and gl_FragColor,
>>         which
>>         have been deprecated since GLSL 1.30, but do not require the
>>         compatibility profile until GLSL 4.20).  We now consistently use
>> the
>>         word "compatibility" to refer to these variables.
>>
>>
>>     This may be evidence of one or more bugs in our implementation.  In
>>     a forward-compatible 3.0+ context, *all* deprecated variables are
>>     removed.  In a 3.1 context without GL_ARB_compatibiliyt, some
>>     deprecated variables are removed.  Which, I think, I mostly what
>>     you're saying.
>>
>>
>> Not exactly.  My reading of the specs is that in a forward-compatible
>> 3.0+ context, all deprecated variables are removed *except*
>> gl_FragColor, gl_FragData, and gl_MaxVaryingFloats.  These three
>> variables are marked as "deprecated" in GLSL 1.30 through 4.10, but the
>> text that says that they are only available in the compatibility profile
>> does not appear until GLSL 4.20.  This looks like a deliberate change
>> rather than a correction of an oversight, since GLSL 4.20 lists
>> gl_FragColor and gl_FragData in its summary of changes since 4.10 as
>> "Move these previously deprecated features to be only in the
>> compatibility profile".
>>
>
> I think we're miscommunicating, so I'll try to clarify. Forward-compatible
> is a special kind of context that the user has to opt-in.  This is
> different than a core profile or 3.1 without GL_ARB_compatibility.  In a
> forward-compatible context every deprecated feature is removed, even if no
> future spec removes it.
>
> So, this means there's really three kinds of context: compatibility
> profile, core profile, and forward-compatible core profile.
>

Ok, thanks.  That cleared up a misconception on my part.  I also spent
about an hour asking Chad questions this afternoon, and that cleared up a
bunch more misconceptions.


>
> Section 1.6 (Deprecation) of the 1.30, 1.40, and 1.50 GLSL specs say:
>
>     "The OpenGL API has a forward compatibility mode that will disallow
>     use of deprecated features. If compiling in a mode where use of
>     deprecated features is disallowed, their use causes compile time
>     errors."
>
> That gl_FragColor and gl_FragData weren't removed in 1.40 (with 3.1
> without GL_ARB_compatibility) or 1.50 (with a 3.2 core profile) doesn't
> matter for this case.  If the user makes a 3.{0,1,2,3} forward-compatible
> context, they are gone because they are deprecated.
>
>        Do we handle the forward-compatible context case correctly?
>>
>>
>> If you mean to ask: "do we do the right thing if the user attempts to
>> compile a pre-GLSL-1.40 shader in a forward-compatible context", then I
>> believe the answer is no.  We go ahead and enable the compatibility-only
>> variables in that case.  With my rewrite, that bug should be trivial to
>> fix.  I'll make a follow-up patch to fix it.  Unfortunately, without my
>> rewrite, it would be a pain to fix.
>>
>
> With the above clarification, what I'm asking is if the user creates a 3.1
> forward-compatible context, does the following shader fail to compile?
>
> #version 140
> void main() { gl_FragColor = vec4(0); }
>

I believe you're right, that there's a bug in our implementation--it would
successfully compile this shader, and it shouldn't.

In fact, based on my discussion with Chad today, it sounds like we don't
have any code at all to modify Mesa's behaviour when the forward-compatible
flag is in effect--my understanding is that the EGL and GLX DRI front-ends
just drop that flag on the floor.  So to fix the bug we would have to do
some plumbing work to get the flag through DRI to Mesa's gl_context.  After
doing that, we would need to do 3 other things:

1. Adjust builtin_variables.cpp to exclude gl_FragColor etc. when the flag
is in effect (easy).
2. Make sure all the deprecated features listed in section E.1 of the 3.1
spec are disabled when the flag is in effect (not sure how much work this
is).
3. Disable any deprecated extensions when the flag is in effect (I know we
take pains not to advertise them via GetString when ctx->API is
API_OPENGL_CORE, but I don't think we actually disable them).

Considering the amount of work involved, I'd prefer if we addressed this
after landing my builtin_variables.cpp rewrite.

Another possibility that Chad suggested when I was talking to him this
afternoon is to just just return BAD_MATCH if the client supplies the
forward-compatibility flag when requesting a 3.0+ context.  Rationale: Mesa
doesn't really support the forward-compatibility flag anyhow (since the EGL
and GLX front-ends just drop it on the floor), and besides we don't know of
any shipping apps that require it (the flag is intended as a developer aid,
so it's unlikely that published apps rely on it).  I don't have a really
strong opinion about is, so I'll let Chad weigh in if he wants to champion
this alternative.


P.S. If I rephrase the bug I *thought* you were asking about, based on my
new understanding of how things work, then it becomes: if the user creates
a 3.1 context without ARB_compatibility support (the only kind of 3.1
context Mesa currently supports), what does the following shader do?

#version 110
void main()
{
  gl_Position = vec4(0.0);
  gl_FrontColor = vec4(0.0); // should be disallowed--requires
ARB_compatibility
}

I believe the answer is, it compiles, and it shouldn't.  Because even
though ARB_compatibility hadn't been invented back when GLSL 1.10 was
published, lack of ARB_compatibility should prevent gl_FrontColor from
being used regardless of what GLSL version the shader is.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20130711/83a15ce8/attachment-0001.html>