[Mesa-dev] [PATCH RFC 0/3] glsl: Rewrite builtin_variables.cpp.

[Paul Berry]

While hunting down some geometry shader bugs last week, I became
concerned about the organization of src/glsl/builtin_variables.cpp,
which is responsible for setting up all the GLSL built-in variables
accessible by a shader, based on the shader type (VS or FS), GLSL
version, desktop/GLES flag, and what extensions are enabled.  I was
concerned about the following things:

- The code contains a separate function to handle each GLSL version
  and shader type (e.g. generate_130_vs_variables to generate the
  vertex shader variables for GLSL 1.30).  Since most GLSL versions
  contain a superset of the variables from the previous version, these
  functions mostly call each other but not always (for example
  generate_130_vs_variables calls generate_120_vs_variables, but
  generate_140_fs_variables doesn't call generate_130_fs_variables).
  As a result, it can be difficult to tell at a glance which set of
  GLSL versions a given function applies to.  Also, it isn't obvious
  which set of functions need to be called to establish the right set
  of variables for a given shader type.  As a result, until very
  recently on my geometry shader branch, uniforms and constants were
  missing from geometry shaders.

- Several variables are declared in multiple places.  For example
  gl_PointCoord (which appears in the fragment shader for every
  version of GLSL except desktop GLSL 1.10) has to be declared in
  three places: builtin_100ES_fs_variables (which applies only to GLSL
  1.00 ES), builtin_300ES_fs_variables (which applies only to GLSL
  3.00 ES), and builtin_120_fs_variables (which applies to all desktop
  GLSL versions 1.20 and onward).

- With the advent of geometry shaders, the level of code duplication
  is going to get a lot worse, because each "varying" variable has to
  be declared once for vertex shaders, twice for geometry shaders (as
  input and as output), and once for fragment shaders.  That's going
  to get worse again in the future when we add tessellation control
  and tessellation evaluation shaders.


This patch series reworks builtin_variables.cpp so that:

- Each built-in variable is declared in exactly one place, and it is
  clear from the surrounding context when each variable is available.
  For example, here's the new declaration of gl_PointCoord:

   if (state->is_version(120, 100))
      add_input(VARYING_SLOT_PNTC, vec2_t, "gl_PointCoord");

- Each varying is declared exactly once.  For example here is the
  declaration of gl_ClipDistance (this covers the VS output, the GS
  input and output, and the FS input):

   if (state->is_version(130, 0)) {
       ADD_VARYING(VARYING_SLOT_CLIP_DIST0, array(float_t, 0),
                   "gl_ClipDistance");
   }

- The code paths are nearly identical for all shader types and GLSL
  versions.

I also made a number of minor changes to improve the readability of
the file.  For example I created a class, builtin_variable_generator,
to keep track of the exec_list of instructions and the
_mesa_glsl_parse_state, so that we don't have to pass those explicitly
around with each function call.

In the process of doing the rewrite, I discovered bugs in gl_TexCoord
and gl_MaxUniformVectors.  I've made separate patches to fix those
bugs, so that they can be cherry-picked to stable branches if
necessary.

This series is avaiable at branch "builtin-variables-rework" of
https://github.com/stereotype441/mesa.git.


[PATCH RFC 1/3] glsl ES: Fix magnitude of gl_MaxVertexUniformVectors.
[PATCH RFC 2/3] glsl: Make gl_TexCoord compatibility-only
[PATCH RFC 3/3] glsl: Rework builtin_variables.cpp to reduce code duplication.