<div dir="ltr">On 11 July 2013 16:30, Ian Romanick <span dir="ltr"><<a href="mailto:idr@freedesktop.org" target="_blank">idr@freedesktop.org</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">

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


<br>
So, this means there's really three kinds of context: compatibility profile, core profile, and forward-compatible core profile.<br></blockquote><div><br></div><div>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.</div>
<div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
Section 1.6 (Deprecation) of the 1.30, 1.40, and 1.50 GLSL specs say:<br>
<br>
    "The OpenGL API has a forward compatibility mode that will disallow<br>
    use of deprecated features. If compiling in a mode where use of<br>
    deprecated features is disallowed, their use causes compile time<br>
    errors."<br>
<br>
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.<br>


<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
      Do we handle the forward-compatible context case correctly?<br>
<br>
<br>
If you mean to ask: "do we do the right thing if the user attempts to<br>
compile a pre-GLSL-1.40 shader in a forward-compatible context", then I<br>
believe the answer is no.  We go ahead and enable the compatibility-only<br>
variables in that case.  With my rewrite, that bug should be trivial to<br>
fix.  I'll make a follow-up patch to fix it.  Unfortunately, without my<br>
rewrite, it would be a pain to fix.<br>
</blockquote>
<br>
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?<br>
<br>
#version 140<br>
void main() { gl_FragColor = vec4(0); }<br></blockquote><div><div><br class="">I believe you're right, that there's a bug in our implementation--it would successfully compile this shader, and it shouldn't.</div>
</div><div><br></div><div>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:</div>
<div><br></div><div>1. Adjust builtin_variables.cpp to exclude gl_FragColor etc. when the flag is in effect (easy).</div><div>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).</div>
<div>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).</div><div>
<br></div><div>Considering the amount of work involved, I'd prefer if we addressed this after landing my builtin_variables.cpp rewrite.</div><div><br></div><div>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.</div>
<div><br></div><div><br></div><div>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?</div>
<div><br></div><div>#version 110</div><div>void main()</div><div>{</div><div>  gl_Position = vec4(0.0);</div><div>  gl_FrontColor = vec4(0.0); // should be disallowed--requires ARB_compatibility</div><div>}</div><div><br>
</div><div>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.</div>
</div></div></div>