<div dir="ltr">Eek, my last reply was too big to be accepted by mesa-dev. Let's try snipping that down and sending it again:<br><div><br><div class="gmail_quote">---------- Forwarded message ----------<br>From: <b class="gmail_sendername">Paul Berry</b> <span dir="ltr"><<a href="mailto:stereotype441@gmail.com">stereotype441@gmail.com</a>></span><br>
Date: 11 July 2013 15:52<br>Subject: Re: [Mesa-dev] [PATCH RFC 3/3] glsl: Rework builtin_variables.cpp to reduce code duplication.<br>To: Ian Romanick <<a href="mailto:idr@freedesktop.org">idr@freedesktop.org</a>><br>
Cc: "<a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a>" <<a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a>><br><br><br><div dir="ltr"><div>
<div class="h5">On 11 July 2013 14:09, Ian Romanick <span dir="ltr"><<a href="mailto:idr@freedesktop.org" target="_blank">idr@freedesktop.org</a>></span> wrote:<br></div></div><div class="gmail_extra"><div class="gmail_quote">
<div><div class="h5"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
I like these changes a lot. I have a few comments below.<div><div><br>
<br>
On 07/08/2013 10:40 AM, Paul Berry wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<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 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, 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>
</blockquote>
<br></div></div>
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.</blockquote>
<div><br></div></div></div><div>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".<br>
</div><div class="im"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> Do we handle the forward-compatible context case correctly?</blockquote>
<div><br>
</div></div><div>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.<br>
</div><div><div class="h5"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div><div><br>
<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
+class builtin_variable_generator<br>
+{<br>
+public:<br>
+ builtin_variable_generator(<u></u>exec_list *instructions,<br>
+ struct _mesa_glsl_parse_state *state);<br>
+ void generate_constants();<br>
+ void generate_uniforms();<br>
+ void generate_vs_special_vars();<br>
+ void generate_gs_special_vars();<br>
+ void generate_fs_special_vars();<br>
+ void generate_varyings();<br>
+<br>
+private:<br>
+ const glsl_type *array(const glsl_type *base, unsigned elements)<br>
+ {<br>
+ return glsl_type::get_array_instance(<u></u>base, elements);<br>
+ }<br>
+ const glsl_type *typ(const char *name)<br>
+ {<br>
+ return symtab->get_type(name);<br>
+ }<br>
</blockquote>
<br></div></div>
Around declarations that are definitions, we generally put a blank line.<div><div><br></div></div></blockquote><div><br></div></div></div><div>Ok, I'll fix this.<br></div><div><div class="h5"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div><div>
<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+ ir_variable *add_variable(const char *name, const glsl_type *type,<br>
+ enum ir_variable_mode mode, int slot);<br>
+ ir_variable *add_uniform(const glsl_type *type, const char *name);<br>
+ ir_variable *add_const(const char *name, int value);<br>
+ ir_variable *add_input(int slot, const glsl_type *type, const char *name)<br>
+ {<br>
+ return add_variable(name, type, ir_var_shader_in, slot);<br>
+ }<br>
+ ir_variable *add_output(int slot, const glsl_type *type, const char *name)<br>
+ {<br>
+ return add_variable(name, type, ir_var_shader_out, slot);<br>
+ }<br>
+ ir_variable *add_system_value(int slot, const glsl_type *type,<br>
+ const char *name)<br>
+ {<br>
+ return add_variable(name, type, ir_var_system_value, slot);<br>
+ }<br>
+ void add_varying(int slot, const glsl_type *type, const char *name,<br>
+ const char *name_as_gs_input);<br>
+<br>
+ exec_list * const instructions;<br>
+ struct _mesa_glsl_parse_state * const state;<br>
+ glsl_symbol_table * const symtab;<br>
+<br>
+ /**<br>
+ * True if compatibility-profile-only variables should be included. (In<br>
+ * desktop GL, these are always included when the GLSL version is 1.30 and<br>
+ * or below).<br>
+ */<br>
+ const bool compatibility;<br>
+<br>
+ const glsl_type * const bool_t;<br>
+ const glsl_type * const int_t;<br>
+ const glsl_type * const float_t;<br>
+ const glsl_type * const vec2_t;<br>
+ const glsl_type * const vec3_t;<br>
+ const glsl_type * const vec4_t;<br>
+ const glsl_type * const mat3_t;<br>
+ const glsl_type * const mat4_t;<br>
</blockquote>
<br></div></div>
I'm not super in love with this. It saves a bit of typing, but it adds yet another way to get at the types. I don't like having to think, "How do I get at the types in *this* source file?"<br>
<br>
If I'm the only objector, I don't think my opposition is strong enough to make you change it.<div><div><br></div></div></blockquote><div><br></div></div></div><div>I don't have a strong opinion either, although for the record I wasn't trying to save typing so much as I was trying to aid in readibility by keeping things on one 80-column line. Without these definitions, 18 of the variable declarations that follow would have to get split to multiple lines, e.g.<br>
<br> ADD_VARYING(VARYING_SLOT_CLIP_VERTEX, vec4_t, "gl_ClipVertex");<br><br></div><div>would become<br><br> ADD_VARYING(VARYING_SLOT_CLIP_VERTEX, glsl_type::vec4_type,<br> "gl_ClipVertex");<br>
<br></div><div>I honestly could go either way.<br></div><div><div class="h5"><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div><div>
<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+};<br>
<br>
-static void<br>
-generate_130_fs_variables(<u></u>exec_list *instructions,<br>
- struct _mesa_glsl_parse_state *state)<br>
+/**<br>
+ * Add a single "varying" variable. The variable's type and direction (input<br>
+ * or output) are adjusted as appropriate for the type of shader being<br>
+ * compiled. For geometry shaders using {ARB,EXT}_geometry_shader4,<br>
+ * name_as_gs_input is used for the input (to avoid ambiguity).<br>
+ */<br>
+void<br>
+builtin_variable_generator::<u></u>add_varying(int slot, const glsl_type *type,<br>
+ const char *name,<br>
+ const char *name_as_gs_input)<br>
{<br>
- generate_120_fs_variables(<u></u>instructions, state, true);<br>
-<br>
- generate_130_uniforms(<u></u>instructions, state);<br>
- generate_fs_clipdistance(<u></u>instructions, state);<br>
+ switch (state->target) {<br>
+ case geometry_shader:<br>
+ add_input(slot, array(type, 0), name_as_gs_input);<br>
+ /* Fall through: */<br>
</blockquote>
<br></div></div>
This needs to be /* FALLTHROUGH */ or /* FALLTHRU */ because some static analysis tools look specifically for one of those strings.</blockquote><div><br></div></div></div><div>Ok, I'll change that.<br></div><div><div class="h5">
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div><div><br>
<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+ case vertex_shader:<br>
+ add_output(slot, type, name);<br>
+ break;<br>
+ case fragment_shader:<br>
+ add_input(slot, type, name);<br>
+ break;<br>
+ }<br>
}<br></blockquote></div></div></blockquote></div></div></div></div></div>
</div><br></div></div>