On Mon, Jun 13, 2016 at 9:08 AM, Ilia Mirkin <imir...@alum.mit.edu> wrote:
> On Mon, Jun 13, 2016 at 12:03 PM, Nanley Chery <nanleych...@gmail.com>
> wrote:
> >
> >
> > On Sun, Jun 12, 2016 at 4:23 PM, Ilia Mirkin <imir...@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 <imir...@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
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
