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

Mon Jun 13 18:28:10 UTC 2016

On Mon, Jun 13, 2016 at 9:08 AM, Ilia Mirkin <imirkin at alum.mit.edu> wrote:

> 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?
>

I don't know enough about the compiler to gauge how it's helped by this
change, but making the _mesa_extension_table[] accessible to it should
be an improvement if we ever want it to iterate through the table in the
future.

And after thinking about the reasons you've stated, I think it makes
sense to keep the changes you've made.

- Nanley

>
>   -ilia
>