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.
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
