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

[Paul Berry]

Eek, my last reply was too big to be accepted by mesa-dev.  Let's try
snipping that down and sending it again:

---------- Forwarded message ----------
From: Paul Berry <stereotype441 at gmail.com>
Date: 11 July 2013 15:52
Subject: Re: [Mesa-dev] [PATCH RFC 3/3] glsl: Rework builtin_variables.cpp
to reduce code duplication.
To: Ian Romanick <idr at freedesktop.org>
Cc: "mesa-dev at lists.freedesktop.org" <mesa-dev at lists.freedesktop.org>


On 11 July 2013 14:09, Ian Romanick <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:
>
>>
>> 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".


>  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.


>
>
>
>> +class builtin_variable_generator
>> +{
>> +public:
>> +   builtin_variable_generator(**exec_list *instructions,
>> +                              struct _mesa_glsl_parse_state *state);
>> +   void generate_constants();
>> +   void generate_uniforms();
>> +   void generate_vs_special_vars();
>> +   void generate_gs_special_vars();
>> +   void generate_fs_special_vars();
>> +   void generate_varyings();
>> +
>> +private:
>> +   const glsl_type *array(const glsl_type *base, unsigned elements)
>> +   {
>> +      return glsl_type::get_array_instance(**base, elements);
>> +   }
>> +   const glsl_type *typ(const char *name)
>> +   {
>> +      return symtab->get_type(name);
>> +   }
>>
>
> Around declarations that are definitions, we generally put a blank line.
>
>
Ok, I'll fix this.


>
>  +   ir_variable *add_variable(const char *name, const glsl_type *type,
>> +                             enum ir_variable_mode mode, int slot);
>> +   ir_variable *add_uniform(const glsl_type *type, const char *name);
>> +   ir_variable *add_const(const char *name, int value);
>> +   ir_variable *add_input(int slot, const glsl_type *type, const char
>> *name)
>> +   {
>> +      return add_variable(name, type, ir_var_shader_in, slot);
>> +   }
>> +   ir_variable *add_output(int slot, const glsl_type *type, const char
>> *name)
>> +   {
>> +      return add_variable(name, type, ir_var_shader_out, slot);
>> +   }
>> +   ir_variable *add_system_value(int slot, const glsl_type *type,
>> +                                 const char *name)
>> +   {
>> +      return add_variable(name, type, ir_var_system_value, slot);
>> +   }
>> +   void add_varying(int slot, const glsl_type *type, const char *name,
>> +                    const char *name_as_gs_input);
>> +
>> +   exec_list * const instructions;
>> +   struct _mesa_glsl_parse_state * const state;
>> +   glsl_symbol_table * const symtab;
>> +
>> +   /**
>> +    * True if compatibility-profile-only variables should be included.
>>  (In
>> +    * desktop GL, these are always included when the GLSL version is
>> 1.30 and
>> +    * or below).
>> +    */
>> +   const bool compatibility;
>> +
>> +   const glsl_type * const bool_t;
>> +   const glsl_type * const int_t;
>> +   const glsl_type * const float_t;
>> +   const glsl_type * const vec2_t;
>> +   const glsl_type * const vec3_t;
>> +   const glsl_type * const vec4_t;
>> +   const glsl_type * const mat3_t;
>> +   const glsl_type * const mat4_t;
>>
>
> 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?"
>
> If I'm the only objector, I don't think my opposition is strong enough to
> make you change it.
>
>
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.

         ADD_VARYING(VARYING_SLOT_CLIP_VERTEX, vec4_t, "gl_ClipVertex");

would become

         ADD_VARYING(VARYING_SLOT_CLIP_VERTEX, glsl_type::vec4_type,
                     "gl_ClipVertex");

I honestly could go either way.



>
>  +};
>>
>> -static void
>> -generate_130_fs_variables(**exec_list *instructions,
>> -                         struct _mesa_glsl_parse_state *state)
>> +/**
>> + * Add a single "varying" variable.  The variable's type and direction
>> (input
>> + * or output) are adjusted as appropriate for the type of shader being
>> + * compiled.  For geometry shaders using {ARB,EXT}_geometry_shader4,
>> + * name_as_gs_input is used for the input (to avoid ambiguity).
>> + */
>> +void
>> +builtin_variable_generator::**add_varying(int slot, const glsl_type
>> *type,
>> +                                        const char *name,
>> +                                        const char *name_as_gs_input)
>>   {
>> -   generate_120_fs_variables(**instructions, state, true);
>> -
>> -   generate_130_uniforms(**instructions, state);
>> -   generate_fs_clipdistance(**instructions, state);
>> +   switch (state->target) {
>> +   case geometry_shader:
>> +      add_input(slot, array(type, 0), name_as_gs_input);
>> +      /* Fall through: */
>>
>
> This needs to be /* FALLTHROUGH */ or /* FALLTHRU */ because some static
> analysis tools look specifically for one of those strings.


Ok, I'll change that.


>
>
>  +   case vertex_shader:
>> +      add_output(slot, type, name);
>> +      break;
>> +   case fragment_shader:
>> +      add_input(slot, type, name);
>> +      break;
>> +   }
>>   }
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20130711/3b1dba03/attachment.html>