[Mesa-dev] [PATCH 3/3] glsl: Drop shader_bit_encoding version checks.

[Paul Berry]

On 13 September 2013 11:25, Kenneth Graunke <kenneth at whitecape.org> wrote:

> We now set the ARB_shader_bit_encoding flag for versions that support
> this functionality, so we don't need to double check it here.
>
> Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
> Cc: Ian Romanick <idr at freedesktop.org>
> ---
>  src/glsl/builtin_functions.cpp | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/src/glsl/builtin_functions.cpp
> b/src/glsl/builtin_functions.cpp
> index c468bd5..b020a7c 100644
> --- a/src/glsl/builtin_functions.cpp
> +++ b/src/glsl/builtin_functions.cpp
> @@ -182,8 +182,7 @@ shader_texture_lod_and_rect(const
> _mesa_glsl_parse_state *state)
>  static bool
>  shader_bit_encoding(const _mesa_glsl_parse_state *state)
>  {
> -   return state->is_version(330, 300) ||
> -          state->ARB_shader_bit_encoding_enable ||
> +   return state->ARB_shader_bit_encoding_enable ||
>            state->ARB_gpu_shader5_enable;
>  }
>
> --
> 1.8.3.4
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>

I'm not a huge fan of this approach.  We're currently inconsistent in Mesa
as to how we handle GLSL extensions that were adopted into later versions
of GLSL (or backports from later versions of GLSL).  For these extensions:

ARB_draw_instanced
ARB_fragment_coord_conventions
ARB_gpu_shader5
ARB_shader_texture_lod
ARB_shading_language_420pack
ARB_shading_language_packing
ARB_texture_cube_map_array
ARB_texture_multisample
OES_texture_3D

we use what I'll call the "enable means explicitly enabled" style, which
means we only set the "_enable" flag if the shader contains text like
"#extension ARB_foo: enable"; if the extension's functionality is provided
by the given version of GLSL, we fold that into the if-test that enables
the functionality--e.g.:

   if (state->ARB_draw_instanced_enable || state->is_version(140, 300))
      add_system_value(SYSTEM_VALUE_INSTANCE_ID, int_t, "gl_InstanceID");

But for these extensions:

ARB_explicit_attrib_location
ARB_texture_rectangle

we use what I'll call the "enable means available" style, which means that
we set the "_enable" flag when processing the version directive (or in the
_mesa_glsl_parse_state constructor), to indicate that the functionality is
available, whether or not the user explicitly requested it or not.

(Note: for ARB_uniform_buffer_object it looks like we do some of each
style!)


Personally I'd prefer to see us consistently adopt the "enable means
explicitly enabled" style (to me, the "enable means available" style feels
like we're fibbing to ourselves).  An additional advantage of the "enable
means explicitly enabled" style is that it allows us to handle cases (such
as in ARB_draw_instanced) where the extension differs slightly from the
functionality that was eventually folded into GLSL.  Another advantage is
that if a client-supplied shader does something silly like "#extension
ARB_draw_instanced: disable" in a GLSL 1.40+ shader, we won't accidentally
disable built-in functionality (although in practice this is extremely
unlikely to ever crop up).

If it becomes too onerous to add the "|| state->is_version(...)" checks all
over the place we can always make some inline functions, e.g.:

struct _mesa_glsl_parse_state {
   ...
   bool is_draw_instanced_available() const
   {
      return this->ARB_draw_instanced_enable || this->is_version(140, 300);
   }
   ...
};
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20130916/6840b391/attachment.html>