[Mesa-dev] [PATCH 2/2] glsl: Rewrote _mesa_glsl_process_extension to use table-driven logic.
Ian Romanick
idr at freedesktop.org
Mon Jun 27 18:30:24 PDT 2011
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
I like this a lot. It's a really good clean up of a rotting cesspool.
I have a few formatting / style comments below and a question.
On 06/27/2011 04:26 PM, Paul Berry wrote:
> Instead of using a chain of manually maintained if/else blocks to
> handle "#extension" directives, we now consult a table that specifies,
> for each extension, the circumstances under which it is available, and
> what flags in _mesa_glsl_parse_state need to be set in order to
> activate it.
>
> This makes it easier to add new GLSL extensions in the future, and
> fixes the following bugs:
>
> - Previously, _mesa_glsl_process_extension would sometimes set the
> "_enable" and "_warn" flags for an extension before checking whether
> the extension was supported by the driver; as a result, specifying
> "enable" behavior for an unsupported extension would sometimes cause
> front-end support for that extension to be switched on in spite of
> the fact that back-end support was not available, leading to strange
> failures, such as those in
> https://bugs.freedesktop.org/show_bug.cgi?id=38015.
>
> - "#extension all: warn" and "#extension all: disable" had no effect.
>
> Notes:
>
> - All extensions are currently marked as unavailable in geometry
> shaders. This should not have any adverse effects since geometry
> shaders aren't supported yet. When we return to working on geometry
> shader support, we'll need to update the table for those extensions
> that are avaiable in geometry shaders.
available
>
> - Previous to this commit, if a shader mentioned
> ARB_shader_texture_lod, extension ARB_texture_rectangle would be
> automatically turned on in order to ensure that the types
> sampler2DRect and sampler2DRectShadow would be defined. This was
> unnecessary, because (a) ARB_shader_texture_lod works perfectly well
> without those types provided that the builtin functions that
> reference them are not called, and (b) ARB_texture_rectangle is
> enabled by default in non-ES contexts anyway. I eliminated this
> unnecessary behavior in order to make the behavior of all extensions
> consistent.
> ---
> src/glsl/glsl_parser_extras.cpp | 281 ++++++++++++++++++++++++---------------
> 1 files changed, 172 insertions(+), 109 deletions(-)
>
> diff --git a/src/glsl/glsl_parser_extras.cpp b/src/glsl/glsl_parser_extras.cpp
> index d9aa300..cdce231 100644
> --- a/src/glsl/glsl_parser_extras.cpp
> +++ b/src/glsl/glsl_parser_extras.cpp
> @@ -165,133 +165,196 @@ _mesa_glsl_warning(const YYLTYPE *locp, _mesa_glsl_parse_state *state,
> }
>
>
> +/**
> + * Enum representing the possible behaviors that can be specified in
> + * an #extension directive.
> + */
> +enum ext_behavior {
> + extension_disable,
> + extension_enable,
> + extension_require,
> + extension_warn
> +};
> +
> +/**
> + * Element type for _mesa_glsl_supported_extensions
> + */
> +struct _mesa_glsl_extension {
> + /* Name of the extension when referred to in a GLSL extension
> + * statement */
Field comments should get the doxygen
/**
* Short description
*
* Detailed description
*/
or
/** Description */
treatment.
Also, we generally prefer the closing */ of a multiline comment to be on
its own line.
> + const char *name;
> +
> + /* True if this extension is available to vertex shaders */
> + bool avail_in_VS;
> +
> + /* True if this extension is available to geometry shaders */
> + bool avail_in_GS;
> +
> + /* True if this extension is available to fragment shaders */
> + bool avail_in_FS;
> +
> + /* True if this extension is available to desktop GL shaders */
> + bool avail_in_GL;
> +
> + /* True if this extension is available to GLES shaders */
> + bool avail_in_ES;
> +
> + /* Flag in the gl_extensions struct indicating whether this
> + * extension is supported by the driver, or
> + * &gl_extensions::dummy_true if supported by all drivers */
> + const GLboolean gl_extensions::* supported_flag;
WTF? Seriously. What does this do? I was expecting to see this code
use the offsetof macro (like src/mesa/main/extensions.c does), but I'm
now suspecting that C++ has some built-in magic for this. Is that true?
> +
> + /* Flag in the _mesa_glsl_parse_state struct that should be set
> + * when this extension is enabled. */
> + bool _mesa_glsl_parse_state::* enable_flag;
> +
> + /* Flag in the _mesa_glsl_parse_state struct that should be set
> + * when the shader requests "warn" behavior for this extension. */
> + bool _mesa_glsl_parse_state::* warn_flag;
> +
> +
> + bool compatible_with_state(const _mesa_glsl_parse_state *state) const;
> + void set_flags(_mesa_glsl_parse_state *state, ext_behavior behavior) const;
> +};
> +
> +#define EXT(NAME, VS, GS, FS, GL, ES, SUPPORTED_FLAG) \
> + { "GL_" #NAME, VS, GS, FS, GL, ES, &gl_extensions::SUPPORTED_FLAG, \
> + &_mesa_glsl_parse_state::NAME##_enable, \
> + &_mesa_glsl_parse_state::NAME##_warn }
> +
> +/**
> + * Table of extensions that can be enabled/disabled within a shader,
> + * and the conditions under which they are supported.
> + */
> +const _mesa_glsl_extension _mesa_glsl_supported_extensions[] = {
This should probably be static.
> + /* target availability API availability */
> + /* name VS GS FS GL ES supported flag */
> + EXT(ARB_draw_buffers, false, false, true, true, false, dummy_true),
> + EXT(ARB_draw_instanced, true, false, false, true, false, ARB_draw_instanced),
> + EXT(ARB_explicit_attrib_location, true, false, true, true, false, ARB_explicit_attrib_location),
> + EXT(ARB_fragment_coord_conventions, true, false, true, true, false, ARB_fragment_coord_conventions),
> + EXT(ARB_texture_rectangle, true, false, true, true, false, dummy_true),
> + EXT(EXT_texture_array, true, false, true, true, false, EXT_texture_array),
> + EXT(ARB_shader_texture_lod, true, false, true, true, false, ARB_shader_texture_lod),
> + EXT(ARB_shader_stencil_export, false, false, true, true, false, ARB_shader_stencil_export),
> + EXT(AMD_conservative_depth, true, false, true, true, false, AMD_conservative_depth),
> + EXT(AMD_shader_stencil_export, false, false, true, true, false, ARB_shader_stencil_export),
> + EXT(OES_texture_3D, true, false, true, false, true, EXT_texture3D),
> +};
> +
> +#undef EXT
> +
> +
> +/**
> + * Determine whether a given extension is compatible with the target,
> + * API, and extension information in the current parser state.
> + */
> +bool _mesa_glsl_extension::compatible_with_state(const _mesa_glsl_parse_state *
> + state) const
> +{
> + /* Check that this extension matches the type of shader we are
> + * compiling to. */
> + switch (state->target) {
> + case vertex_shader: if (!this->avail_in_VS) return false; break;
> + case geometry_shader: if (!this->avail_in_GS) return false; break;
> + case fragment_shader: if (!this->avail_in_FS) return false; break;
Delete the spurious breaks.
> + default:
> + assert (!"Unrecognized shader target");
> + return false;
> + }
> +
> + /* Check that this extension matches whether we are compiling
> + * for desktop GL or GLES. */
> + if (state->es_shader) {
> + if (!this->avail_in_ES) return false;
> + } else {
> + if (!this->avail_in_GL) return false;
> + }
> +
> + /* Check that this extension is supported by the OpenGL
> + * implementation. */
> + return state->extensions->*(this->supported_flag);
> +}
> +
> +/**
> + * Set the appropriate flags in the parser state to establish the
> + * given behavior for this extension.
> + */
> +void _mesa_glsl_extension::set_flags(_mesa_glsl_parse_state *state,
> + ext_behavior behavior) const
> +{
> + state->*(this->enable_flag) = (behavior != extension_disable);
> + state->*(this->warn_flag) = (behavior == extension_warn);
> +}
> +
> +/**
> + * Find an extension by name in _mesa_glsl_supported_extensions. If
> + * the name is not found, return NULL.
> + */
> +static const _mesa_glsl_extension *find_extension(const char *name)
> +{
> + for (unsigned int i = 0; i < Elements(_mesa_glsl_supported_extensions);
> + ++i) {
Just 'unsigned'.
> + if (strcmp(name, _mesa_glsl_supported_extensions[i].name) == 0) {
> + return &_mesa_glsl_supported_extensions[i];
> + }
> + }
> + return NULL;
> +}
> +
> +
> bool
> _mesa_glsl_process_extension(const char *name, YYLTYPE *name_locp,
> - const char *behavior, YYLTYPE *behavior_locp,
> + const char *behavior_string, YYLTYPE *behavior_locp,
> _mesa_glsl_parse_state *state)
> {
> - enum {
> - extension_disable,
> - extension_enable,
> - extension_require,
> - extension_warn
> - } ext_mode;
> -
> - if (strcmp(behavior, "warn") == 0) {
> - ext_mode = extension_warn;
> - } else if (strcmp(behavior, "require") == 0) {
> - ext_mode = extension_require;
> - } else if (strcmp(behavior, "enable") == 0) {
> - ext_mode = extension_enable;
> - } else if (strcmp(behavior, "disable") == 0) {
> - ext_mode = extension_disable;
> + ext_behavior behavior;
> + if (strcmp(behavior_string, "warn") == 0) {
> + behavior = extension_warn;
> + } else if (strcmp(behavior_string, "require") == 0) {
> + behavior = extension_require;
> + } else if (strcmp(behavior_string, "enable") == 0) {
> + behavior = extension_enable;
> + } else if (strcmp(behavior_string, "disable") == 0) {
> + behavior = extension_disable;
> } else {
> _mesa_glsl_error(behavior_locp, state,
> "Unknown extension behavior `%s'",
> - behavior);
> + behavior_string);
> return false;
> }
>
> - bool unsupported = false;
> -
> if (strcmp(name, "all") == 0) {
> - if ((ext_mode == extension_enable) || (ext_mode == extension_require)) {
> + if ((behavior == extension_enable) || (behavior == extension_require)) {
> _mesa_glsl_error(name_locp, state, "Cannot %s all extensions",
> - (ext_mode == extension_enable)
> + (behavior == extension_enable)
> ? "enable" : "require");
> return false;
> - }
> - } else if (strcmp(name, "GL_ARB_draw_buffers") == 0) {
> - /* This extension is only supported in fragment shaders.
> - */
> - if (state->target != fragment_shader) {
> - unsupported = true;
> } else {
> - state->ARB_draw_buffers_enable = (ext_mode != extension_disable);
> - state->ARB_draw_buffers_warn = (ext_mode == extension_warn);
> + for (unsigned int i = 0;
Same as above.
> + i < Elements(_mesa_glsl_supported_extensions); ++i) {
> + const _mesa_glsl_extension *extension
> + = &_mesa_glsl_supported_extensions[i];
> + if (extension->compatible_with_state(state)) {
> + _mesa_glsl_supported_extensions[i].set_flags(state, behavior);
extension->set_flags(state, behavior);
> + }
> + }
> }
> - } else if (strcmp(name, "GL_ARB_draw_instanced") == 0) {
> - state->ARB_draw_instanced_enable = (ext_mode != extension_disable);
> - state->ARB_draw_instanced_warn = (ext_mode == extension_warn);
> -
> - /* This extension is only supported in vertex shaders.
> - */
> - unsupported = (state->target != vertex_shader)
> - || !state->extensions->ARB_draw_instanced;
> - } else if (strcmp(name, "GL_ARB_explicit_attrib_location") == 0) {
> - state->ARB_explicit_attrib_location_enable =
> - (ext_mode != extension_disable);
> - state->ARB_explicit_attrib_location_warn =
> - (ext_mode == extension_warn);
> -
> - unsupported = !state->extensions->ARB_explicit_attrib_location;
> - } else if (strcmp(name, "GL_ARB_fragment_coord_conventions") == 0) {
> - state->ARB_fragment_coord_conventions_enable =
> - (ext_mode != extension_disable);
> - state->ARB_fragment_coord_conventions_warn =
> - (ext_mode == extension_warn);
> -
> - unsupported = !state->extensions->ARB_fragment_coord_conventions;
> - } else if (strcmp(name, "GL_ARB_texture_rectangle") == 0) {
> - state->ARB_texture_rectangle_enable = (ext_mode != extension_disable);
> - state->ARB_texture_rectangle_warn = (ext_mode == extension_warn);
> - } else if (strcmp(name, "GL_EXT_texture_array") == 0) {
> - state->EXT_texture_array_enable = (ext_mode != extension_disable);
> - state->EXT_texture_array_warn = (ext_mode == extension_warn);
> -
> - unsupported = !state->extensions->EXT_texture_array;
> - } else if (strcmp(name, "GL_ARB_shader_texture_lod") == 0) {
> - /* Force ARB_texture_rectangle to be on so sampler2DRects are defined */
> - state->ARB_texture_rectangle_enable = true;
> -
> - state->ARB_shader_texture_lod_enable = (ext_mode != extension_disable);
> - state->ARB_shader_texture_lod_warn = (ext_mode == extension_warn);
> -
> - unsupported = !state->extensions->ARB_shader_texture_lod;
> - } else if (strcmp(name, "GL_ARB_shader_stencil_export") == 0) {
> - state->ARB_shader_stencil_export_enable = (ext_mode != extension_disable);
> - state->ARB_shader_stencil_export_warn = (ext_mode == extension_warn);
> -
> - /* This extension is only supported in fragment shaders.
> - */
> - unsupported = (state->target != fragment_shader)
> - || !state->extensions->ARB_shader_stencil_export;
> - } else if (strcmp(name, "GL_AMD_conservative_depth") == 0) {
> - /* The AMD_conservative spec does not forbid requiring the extension in
> - * the vertex shader.
> - */
> - state->AMD_conservative_depth_enable = (ext_mode != extension_disable);
> - state->AMD_conservative_depth_warn = (ext_mode == extension_warn);
> - unsupported = !state->extensions->AMD_conservative_depth;
> - } else if (strcmp(name, "GL_AMD_shader_stencil_export") == 0) {
> - state->AMD_shader_stencil_export_enable = (ext_mode != extension_disable);
> - state->AMD_shader_stencil_export_warn = (ext_mode == extension_warn);
> -
> - /* This extension is only supported in fragment shaders.
> - * Both the ARB and AMD variants share the same ARB flag
> - * in gl_extensions.
> - */
> - unsupported = (state->target != fragment_shader)
> - || !state->extensions->ARB_shader_stencil_export;
> - } else if (strcmp(name, "GL_OES_texture_3D") == 0 && state->es_shader) {
> - state->OES_texture_3D_enable = (ext_mode != extension_disable);
> - state->OES_texture_3D_warn = (ext_mode == extension_warn);
> -
> - unsupported = !state->extensions->EXT_texture3D;
> } else {
> - unsupported = true;
> - }
> -
> - if (unsupported) {
> - static const char *const fmt = "extension `%s' unsupported in %s shader";
> -
> - if (ext_mode == extension_require) {
> - _mesa_glsl_error(name_locp, state, fmt,
> - name, _mesa_glsl_shader_target_name(state->target));
> - return false;
> + const _mesa_glsl_extension *extension = find_extension(name);
> + if (extension && extension->compatible_with_state(state)) {
> + extension->set_flags(state, behavior);
> } else {
> - _mesa_glsl_warning(name_locp, state, fmt,
> - name, _mesa_glsl_shader_target_name(state->target));
> + static const char *const fmt = "extension `%s' unsupported in %s shader";
> +
> + if (behavior == extension_require) {
> + _mesa_glsl_error(name_locp, state, fmt,
> + name, _mesa_glsl_shader_target_name(state->target));
> + return false;
> + } else {
> + _mesa_glsl_warning(name_locp, state, fmt,
> + name, _mesa_glsl_shader_target_name(state->target));
> + }
> }
> }
>
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
iEYEARECAAYFAk4JLrAACgkQX1gOwKyEAw/yRgCfbTX65N/LtxByhGLkCREy2RCv
oaAAnRFPVvyCmI1+zdCUnyC0MLtoIjMr
=cx/M
-----END PGP SIGNATURE-----
More information about the mesa-dev
mailing list