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

[Ian Romanick]

On 09/16/2013 04:20 PM, Paul Berry wrote:
> On 13 September 2013 11:25, Kenneth Graunke <kenneth at whitecape.org
> <mailto: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
>     <mailto:kenneth at whitecape.org>>
>     Cc: Ian Romanick <idr at freedesktop.org <mailto: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 <mailto: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

This extension is slightly special because of lolz.

> 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);
>    }
>    ...
> };

If we /is/has/, then I like this approach.  I also think we should use
it for all extension functionality in the compiler.  We currently have a
problem that a dumb shader like the following will do the wrong thing:

#version 330
#extension GL_ARB_explicit_attrib_location: disable

layout(location=0) in vec4 v;

It's not a big deal, but it has bugged me for years.

> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev