[Mesa-dev] [PATCH] glsl: reuse main extension table to appropriate restrict extensions

[Ilia Mirkin]

On Mon, Jun 13, 2016 at 12:03 PM, Nanley Chery <nanleychery at gmail.com> wrote:
>
>
> On Sun, Jun 12, 2016 at 4:23 PM, Ilia Mirkin <imirkin at alum.mit.edu> wrote:
>>
>> Previously we were only restricting based on ES/non-ES-ness and whether
>> the overall enable bit had been flipped on. However we have been adding
>> more fine-grained restrictions, such as based on compat profiles, as
>> well as specific ES versions. Most of the time this doesn't matter, but
>> it can create awkward situations and duplication of logic.
>>
>> Here we separate the main extension table into a separate object file,
>> linked to the glsl compiler, which makes use of it with a custom
>> function which takes the ES-ness of the shader into account (thus
>> allowing desktop shaders to properly use ES extensions that would
>> otherwise have been disallowed.)
>>
>> The effect of this change should be nil in most cases.
>>
>> Signed-off-by: Ilia Mirkin <imirkin at alum.mit.edu>
>> ---
>>  src/Makefile.am                          |   1 +
>>  src/compiler/SConscript.glsl             |   2 +
>>  src/compiler/glsl/glsl_parser_extras.cpp | 238
>> ++++++++++++++++---------------
>>  src/mesa/Android.libmesa_glsl_utils.mk   |   2 +
>>  src/mesa/Makefile.sources                |   1 +
>>  src/mesa/main/extensions.c               |  30 ++--
>>  src/mesa/main/extensions_table.c         |  51 +++++++
>>  7 files changed, 191 insertions(+), 134 deletions(-)
>>  create mode 100644 src/mesa/main/extensions_table.c
>>
>> diff --git a/src/Makefile.am b/src/Makefile.am
>> index 0527a31..a749bf6 100644
>> --- a/src/Makefile.am
>> +++ b/src/Makefile.am
>> @@ -114,6 +114,7 @@ AM_CPPFLAGS = \
>>  noinst_LTLIBRARIES = libglsl_util.la
>>
>>  libglsl_util_la_SOURCES = \
>> +       mesa/main/extensions_table.c \
>>         mesa/main/imports.c \
>>         mesa/program/prog_hash_table.c \
>>         mesa/program/symbol_table.c \
>> diff --git a/src/compiler/SConscript.glsl b/src/compiler/SConscript.glsl
>> index 4252ce1..31d8f6d 100644
>> --- a/src/compiler/SConscript.glsl
>> +++ b/src/compiler/SConscript.glsl
>> @@ -70,6 +70,7 @@ if env['msvc']:
>>  # Copy these files to avoid generation object files into src/mesa/program
>>  env.Prepend(CPPPATH = ['#src/mesa/main'])
>>  env.Command('glsl/imports.c', '#src/mesa/main/imports.c', Copy('$TARGET',
>> '$SOURCE'))
>> +env.Command('glsl/extensions_table.c',
>> '#src/mesa/main/extensions_table.c', Copy('$TARGET', '$SOURCE'))
>>  # Copy these files to avoid generation object files into src/mesa/program
>>  env.Prepend(CPPPATH = ['#src/mesa/program'])
>>  env.Command('glsl/prog_hash_table.c',
>> '#src/mesa/program/prog_hash_table.c', Copy('$TARGET', '$SOURCE'))
>> @@ -79,6 +80,7 @@ env.Command('glsl/dummy_errors.c',
>> '#src/mesa/program/dummy_errors.c', Copy('$TA
>>  compiler_objs = env.StaticObject(source_lists['GLSL_COMPILER_CXX_FILES'])
>>
>>  mesa_objs = env.StaticObject([
>> +    'glsl/extensions_table.c',
>>      'glsl/imports.c',
>>      'glsl/prog_hash_table.c',
>>      'glsl/symbol_table.c',
>> diff --git a/src/compiler/glsl/glsl_parser_extras.cpp
>> b/src/compiler/glsl/glsl_parser_extras.cpp
>> index ce2c3e8..1f65412 100644
>> --- a/src/compiler/glsl/glsl_parser_extras.cpp
>> +++ b/src/compiler/glsl/glsl_parser_extras.cpp
>> @@ -510,28 +510,12 @@ struct _mesa_glsl_extension {
>>      */
>>     const char *name;
>>
>> -   /** 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.
>> -    *
>> -    * Note: the type (GLboolean gl_extensions::*) is a "pointer to
>> -    * member" type, the type-safe alternative to the "offsetof" macro.
>> -    * In a nutshell:
>> -    *
>> -    * - foo bar::* p declares p to be an "offset" to a field of type
>> -    *   foo that exists within struct bar
>> -    * - &bar::baz computes the "offset" of field baz within struct bar
>> -    * - x.*p accesses the field of x that exists at "offset" p
>> -    * - x->*p is equivalent to (*x).*p
>> +    * Predicate that checks whether the relevant extension is available
>> for
>> +    * this context.
>>      */
>> -   const GLboolean gl_extensions::* supported_flag;
>> +   bool (*available_pred)(const struct gl_context *,
>> +                          gl_api api, uint8_t version);
>>
>>     /**
>>      * Flag in the _mesa_glsl_parse_state struct that should be set
>> @@ -556,8 +540,19 @@ struct _mesa_glsl_extension {
>>     void set_flags(_mesa_glsl_parse_state *state, ext_behavior behavior)
>> const;
>>  };
>>
>> -#define EXT(NAME, GL, ES, SUPPORTED_FLAG)                   \
>> -   { "GL_" #NAME, GL, ES, &gl_extensions::SUPPORTED_FLAG,   \
>> +/** Checks if the context supports a user-facing extension */
>> +#define EXT(name_str, driver_cap, ...) \
>> +static MAYBE_UNUSED bool \
>> +has_##name_str(const struct gl_context *ctx, gl_api api, uint8_t version)
>> \
>> +{ \
>> +   return ctx->Extensions.driver_cap && (version >= \
>> +          _mesa_extension_table[MESA_EXTENSION_##name_str].version[api]);
>> \
>> +}
>
>
> This looks like a nice clean up. If I have some spare cycles, I may be able
> to review
>  it in its entirety.
>
> I think you can simplify this patch by making a helper that doesn't pull in
> _mesa_extension_table[]. This was something I overlooked while creating the
> helpers in extensions.h. You can do this by defining the EXT macro to
> contain each
> GL/ES version and then create an anonymous array that the api variable
> indexes.

This patch went through a few iterations. Originally I was using the
_mesa_has_NAME() helpers (which required piping through the extension
table), but then I realized that I'd have to have a custom helper
anyways.

However it's more compact to just have the extension table in one
place, and now that I've done the work to have it in the compiler,
might as well use it? What do you think?

  -ilia