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

2016-06-13 Thread Ilia Mirkin 
On Mon, Jun 13, 2016 at 1:27 PM, Eric Engestrom
 wrote:
> On Mon, Jun 13, 2016 at 09:39:39AM -0700, Nanley Chery wrote:
>> You can add an entry to the end of the enum defined in extensions.h called
>> "MESA_EXTENSION_COUNT"  or similar.
>
> This is a good idea, but with one caveat:
> I've seen twice already bugs caused by people using ALL_CAPS counter
> entries in enums, and then someone trying to use them in #if conditions,
> where it resolves as `0` since the preprocessor doesn't know them.
>
> I would strongly advise to use lower case names, so as to avoid this
> confusion.

I see where you're coming from, but that's not the convention in mesa.
The enums are already MESA_EXTENSION_ARB_foo, so it'd be odd to call
the last thing "mesa_extension_count". I also can't think of a legit
scenario where someone would want to use MESA_EXTENSION_COUNT in a
preprocessor macro. So I'm going to leave it upper-cased.

  -ilia
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

2016-06-13 Thread Nanley Chery 


On Mon, Jun 13, 2016 at 9:08 AM, Ilia Mirkin  wrote:

> On Mon, Jun 13, 2016 at 12:03 PM, Nanley Chery  
> wrote:
> >
> >
> > On Sun, Jun 12, 2016 at 4:23 PM, Ilia Mirkin  
> 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 
> >> ---
> >>  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
> >> -* _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
> >> -* - ::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 {
> >>

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

2016-06-13 Thread Eric Engestrom 
On Mon, Jun 13, 2016 at 09:39:39AM -0700, Nanley Chery wrote:
> You can add an entry to the end of the enum defined in extensions.h called
> "MESA_EXTENSION_COUNT"  or similar.

This is a good idea, but with one caveat:
I've seen twice already bugs caused by people using ALL_CAPS counter
entries in enums, and then someone trying to use them in #if conditions,
where it resolves as `0` since the preprocessor doesn't know them.

I would strongly advise to use lower case names, so as to avoid this
confusion.

Cheers,
  Eric
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

2016-06-13 Thread Ilia Mirkin 
On Mon, Jun 13, 2016 at 12:39 PM, Nanley Chery  wrote:
>
>
> On Mon, Jun 13, 2016 at 9:16 AM, Ilia Mirkin  wrote:
>>
>> On Mon, Jun 13, 2016 at 12:11 PM, Emil Velikov 
>> wrote:
>> > Hi Ilia,
>> >
>> > On 13 June 2016 at 00:23, Ilia Mirkin  wrote:
>> >
>> >> @@ -81,6 +81,7 @@ MAIN_FILES = \
>> >> main/execmem.c \
>> >> main/extensions.c \
>> >> main/extensions.h \
>> >> +   main/extensions_table.c \
>> > From a build perspective everything looks amazing. Thank you !
>>
>> Thanks for checking.
>>
>> >
>> >
>> >> --- a/src/mesa/main/extensions.c
>> >> +++ b/src/mesa/main/extensions.c
>> >> @@ -49,25 +49,15 @@ static char *extra_extensions = NULL;
>> >>  #define o(x) offsetof(struct gl_extensions, x)
>> >>
>> >>
>> >> -/**
>> >> - * \brief Table of supported OpenGL extensions for all API's.
>> >> - */
>> >> -const struct mesa_extension _mesa_extension_table[] = {
>> >> +static bool extension_table_size[] = {
>> >>  #define EXT(name_str, driver_cap, gll_ver, glc_ver, gles_ver,
>> >> gles2_ver, ) \
>> >> -{ .name = "GL_" #name_str, .offset = o(driver_cap), \
>> >> -  .version = { \
>> >> -[API_OPENGL_COMPAT] = gll_ver, \
>> >> -[API_OPENGL_CORE]   = glc_ver, \
>> >> -[API_OPENGLES]  = gles_ver, \
>> >> -[API_OPENGLES2] = gles2_ver, \
>> >> -   }, \
>> >> -   .year =  \
>> >> -},
>> >> +   0,
>> >> +
>> >>  #include "extensions_table.h"
>> >>  #undef EXT
>> >>  };
>> >>
>> > An alternative idea to the one proposed by Eric:
>> > Explicitly set the array size (ideally as a macro in the relevant
>> > header) and use that instead of having a dummy array, only to know the
>> > array size.
>>
>> I couldn't come up with a non-horrible way of doing that. Are you
>> basically suggesting I add a
>>
>> #define EXTENSIONS_COUNT 1
>>
>> in extensions_table.h, and expect someone to increment it every time
>> an extension was being added?
>>
>> I guess it could be enforced via a STATIC_ASSERT in the new
>> extensions_table.c, but it feels really dirty. I guess a cleverer
>> thing would be to count by doing something horrible for the compiler,
>> e.g.
>>
>> static const int foo = 0 +1+1+1+1+1+1..
>>
>> Probably nicer than my array of 0's though. Open to other ideas.
>>
>
> You can add an entry to the end of the enum defined in extensions.h called
> "MESA_EXTENSION_COUNT"  or similar.

Duh. Of course. Much better!
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

2016-06-13 Thread Emil Velikov 
On 13 June 2016 at 17:16, Ilia Mirkin  wrote:
> On Mon, Jun 13, 2016 at 12:11 PM, Emil Velikov  
> wrote:
>> Hi Ilia,
>>
>> On 13 June 2016 at 00:23, Ilia Mirkin  wrote:
>>
>>> @@ -81,6 +81,7 @@ MAIN_FILES = \
>>> main/execmem.c \
>>> main/extensions.c \
>>> main/extensions.h \
>>> +   main/extensions_table.c \
>> From a build perspective everything looks amazing. Thank you !
>
> Thanks for checking.
>
>>
>>
>>> --- a/src/mesa/main/extensions.c
>>> +++ b/src/mesa/main/extensions.c
>>> @@ -49,25 +49,15 @@ static char *extra_extensions = NULL;
>>>  #define o(x) offsetof(struct gl_extensions, x)
>>>
>>>
>>> -/**
>>> - * \brief Table of supported OpenGL extensions for all API's.
>>> - */
>>> -const struct mesa_extension _mesa_extension_table[] = {
>>> +static bool extension_table_size[] = {
>>>  #define EXT(name_str, driver_cap, gll_ver, glc_ver, gles_ver, gles2_ver, 
>>> ) \
>>> -{ .name = "GL_" #name_str, .offset = o(driver_cap), \
>>> -  .version = { \
>>> -[API_OPENGL_COMPAT] = gll_ver, \
>>> -[API_OPENGL_CORE]   = glc_ver, \
>>> -[API_OPENGLES]  = gles_ver, \
>>> -[API_OPENGLES2] = gles2_ver, \
>>> -   }, \
>>> -   .year =  \
>>> -},
>>> +   0,
>>> +
>>>  #include "extensions_table.h"
>>>  #undef EXT
>>>  };
>>>
>> An alternative idea to the one proposed by Eric:
>> Explicitly set the array size (ideally as a macro in the relevant
>> header) and use that instead of having a dummy array, only to know the
>> array size.
>
> I couldn't come up with a non-horrible way of doing that. Are you
> basically suggesting I add a
>
> #define EXTENSIONS_COUNT 1
>
> in extensions_table.h, and expect someone to increment it every time
> an extension was being added?
>
Yes, pretty much (with the correct count of course)

> I guess it could be enforced via a STATIC_ASSERT in the new
> extensions_table.c, but it feels really dirty.
Personally a big fat warning at the top of the file + a STATIC_ASSERT
feels like a more elegant solution.

Even without the STATIC_ASSERT the compiler gives us a nice warning as
we attempt to assign more data than the actual array size.

> I guess a cleverer
> thing would be to count by doing something horrible for the compiler,
> e.g.
>
> static const int foo = 0 +1+1+1+1+1+1..
>
The following does work, although it does feel a bit nasty.

static const int foo =
#define EXT(...) 1 +
#include far.h
0;

> Probably nicer than my array of 0's though. Open to other ideas.
>
At the end of the day, it's not my call so feel free to go with
whichever, as long as others are happy.

-Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

2016-06-13 Thread Nanley Chery 
On Mon, Jun 13, 2016 at 9:16 AM, Ilia Mirkin  wrote:

> On Mon, Jun 13, 2016 at 12:11 PM, Emil Velikov 
> wrote:
> > Hi Ilia,
> >
> > On 13 June 2016 at 00:23, Ilia Mirkin  wrote:
> >
> >> @@ -81,6 +81,7 @@ MAIN_FILES = \
> >> main/execmem.c \
> >> main/extensions.c \
> >> main/extensions.h \
> >> +   main/extensions_table.c \
> > From a build perspective everything looks amazing. Thank you !
>
> Thanks for checking.
>
> >
> >
> >> --- a/src/mesa/main/extensions.c
> >> +++ b/src/mesa/main/extensions.c
> >> @@ -49,25 +49,15 @@ static char *extra_extensions = NULL;
> >>  #define o(x) offsetof(struct gl_extensions, x)
> >>
> >>
> >> -/**
> >> - * \brief Table of supported OpenGL extensions for all API's.
> >> - */
> >> -const struct mesa_extension _mesa_extension_table[] = {
> >> +static bool extension_table_size[] = {
> >>  #define EXT(name_str, driver_cap, gll_ver, glc_ver, gles_ver,
> gles2_ver, ) \
> >> -{ .name = "GL_" #name_str, .offset = o(driver_cap), \
> >> -  .version = { \
> >> -[API_OPENGL_COMPAT] = gll_ver, \
> >> -[API_OPENGL_CORE]   = glc_ver, \
> >> -[API_OPENGLES]  = gles_ver, \
> >> -[API_OPENGLES2] = gles2_ver, \
> >> -   }, \
> >> -   .year =  \
> >> -},
> >> +   0,
> >> +
> >>  #include "extensions_table.h"
> >>  #undef EXT
> >>  };
> >>
> > An alternative idea to the one proposed by Eric:
> > Explicitly set the array size (ideally as a macro in the relevant
> > header) and use that instead of having a dummy array, only to know the
> > array size.
>
> I couldn't come up with a non-horrible way of doing that. Are you
> basically suggesting I add a
>
> #define EXTENSIONS_COUNT 1
>
> in extensions_table.h, and expect someone to increment it every time
> an extension was being added?
>
> I guess it could be enforced via a STATIC_ASSERT in the new
> extensions_table.c, but it feels really dirty. I guess a cleverer
> thing would be to count by doing something horrible for the compiler,
> e.g.
>
> static const int foo = 0 +1+1+1+1+1+1..
>
> Probably nicer than my array of 0's though. Open to other ideas.
>
>
You can add an entry to the end of the enum defined in extensions.h called
"MESA_EXTENSION_COUNT"  or similar.

- Nanley


>   -ilia
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

2016-06-13 Thread Eric Engestrom 
On Mon, Jun 13, 2016 at 12:08:24PM +0100, Eric Engestrom wrote:
> 
> > diff --git a/src/mesa/main/extensions_table.c 
> > b/src/mesa/main/extensions_table.c
> > new file mode 100644
> > index 000..1e37fbc
> > --- /dev/null
> > +++ b/src/mesa/main/extensions_table.c
> > @@ -0,0 +1,51 @@
> > +/*
> > + * Mesa 3-D graphics library
> > + *
> > + * Copyright (C) 1999-2008  Brian Paul   All Rights Reserved.
> > + * Copyright (C) 2009  VMware, Inc.  All Rights Reserved.
> > + *
> > + * Permission is hereby granted, free of charge, to any person obtaining a
> > + * copy of this software and associated documentation files (the 
> > "Software"),
> > + * to deal in the Software without restriction, including without 
> > limitation
> > + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> > + * and/or sell copies of the Software, and to permit persons to whom the
> > + * Software is furnished to do so, subject to the following conditions:
> > + *
> > + * The above copyright notice and this permission notice shall be included
> > + * in all copies or substantial portions of the Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
> > + * OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF 
> > MERCHANTABILITY,
> > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR
> > + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> > + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> > + * OTHER DEALINGS IN THE SOFTWARE.
> > + */
> > +
> > +#include "main/mtypes.h"
> > +#include "main/extensions.h"
> > +
> > +/**
> > + * Given a member \c x of struct gl_extensions, return offset of
> > + * \c x in bytes.
> > + */
> > +#define o(x) offsetof(struct gl_extensions, x)
> > +
> > +/**
> > + * \brief Table of supported OpenGL extensions for all API's.
> > + */
> > +const struct mesa_extension _mesa_extension_table[] = {
> > +#define EXT(name_str, driver_cap, gll_ver, glc_ver, gles_ver, gles2_ver, 
> > ) \
> > +{ .name = "GL_" #name_str, .offset = o(driver_cap), \
> > +  .version = { \
> > +[API_OPENGL_COMPAT] = gll_ver, \
> > +[API_OPENGL_CORE]   = glc_ver, \
> > +[API_OPENGLES]  = gles_ver, \
> > +[API_OPENGLES2] = gles2_ver, \
> > +   }, \
> > +   .year =  \
> > +},
> > +#include "extensions_table.h"
> > +#undef EXT
> 
> I'm not sure this macro is useful in the first place, but:
> 
>   #undef o

Second errata:
I just realised this is a .c, not a .h, so the dangling #define doesn't matter.
Please disregard my earlier comment about it, and I apologise :)
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

2016-06-13 Thread Ilia Mirkin 
On Mon, Jun 13, 2016 at 12:11 PM, Emil Velikov  wrote:
> Hi Ilia,
>
> On 13 June 2016 at 00:23, Ilia Mirkin  wrote:
>
>> @@ -81,6 +81,7 @@ MAIN_FILES = \
>> main/execmem.c \
>> main/extensions.c \
>> main/extensions.h \
>> +   main/extensions_table.c \
> From a build perspective everything looks amazing. Thank you !

Thanks for checking.

>
>
>> --- a/src/mesa/main/extensions.c
>> +++ b/src/mesa/main/extensions.c
>> @@ -49,25 +49,15 @@ static char *extra_extensions = NULL;
>>  #define o(x) offsetof(struct gl_extensions, x)
>>
>>
>> -/**
>> - * \brief Table of supported OpenGL extensions for all API's.
>> - */
>> -const struct mesa_extension _mesa_extension_table[] = {
>> +static bool extension_table_size[] = {
>>  #define EXT(name_str, driver_cap, gll_ver, glc_ver, gles_ver, gles2_ver, 
>> ) \
>> -{ .name = "GL_" #name_str, .offset = o(driver_cap), \
>> -  .version = { \
>> -[API_OPENGL_COMPAT] = gll_ver, \
>> -[API_OPENGL_CORE]   = glc_ver, \
>> -[API_OPENGLES]  = gles_ver, \
>> -[API_OPENGLES2] = gles2_ver, \
>> -   }, \
>> -   .year =  \
>> -},
>> +   0,
>> +
>>  #include "extensions_table.h"
>>  #undef EXT
>>  };
>>
> An alternative idea to the one proposed by Eric:
> Explicitly set the array size (ideally as a macro in the relevant
> header) and use that instead of having a dummy array, only to know the
> array size.

I couldn't come up with a non-horrible way of doing that. Are you
basically suggesting I add a

#define EXTENSIONS_COUNT 1

in extensions_table.h, and expect someone to increment it every time
an extension was being added?

I guess it could be enforced via a STATIC_ASSERT in the new
extensions_table.c, but it feels really dirty. I guess a cleverer
thing would be to count by doing something horrible for the compiler,
e.g.

static const int foo = 0 +1+1+1+1+1+1..

Probably nicer than my array of 0's though. Open to other ideas.

  -ilia
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

2016-06-13 Thread Emil Velikov 
Hi Ilia,

On 13 June 2016 at 00:23, Ilia Mirkin  wrote:

> @@ -81,6 +81,7 @@ MAIN_FILES = \
> main/execmem.c \
> main/extensions.c \
> main/extensions.h \
> +   main/extensions_table.c \
From a build perspective everything looks amazing. Thank you !


> --- a/src/mesa/main/extensions.c
> +++ b/src/mesa/main/extensions.c
> @@ -49,25 +49,15 @@ static char *extra_extensions = NULL;
>  #define o(x) offsetof(struct gl_extensions, x)
>
>
> -/**
> - * \brief Table of supported OpenGL extensions for all API's.
> - */
> -const struct mesa_extension _mesa_extension_table[] = {
> +static bool extension_table_size[] = {
>  #define EXT(name_str, driver_cap, gll_ver, glc_ver, gles_ver, gles2_ver, 
> ) \
> -{ .name = "GL_" #name_str, .offset = o(driver_cap), \
> -  .version = { \
> -[API_OPENGL_COMPAT] = gll_ver, \
> -[API_OPENGL_CORE]   = glc_ver, \
> -[API_OPENGLES]  = gles_ver, \
> -[API_OPENGLES2] = gles2_ver, \
> -   }, \
> -   .year =  \
> -},
> +   0,
> +
>  #include "extensions_table.h"
>  #undef EXT
>  };
>
An alternative idea to the one proposed by Eric:
Explicitly set the array size (ideally as a macro in the relevant
header) and use that instead of having a dummy array, only to know the
array size.

-Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

2016-06-13 Thread Ilia Mirkin 
On Mon, Jun 13, 2016 at 12:03 PM, Nanley Chery  wrote:
>
>
> On Sun, Jun 12, 2016 at 4:23 PM, Ilia Mirkin  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 
>> ---
>>  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
>> -* _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
>> -* - ::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, _extensions::SUPPORTED_FLAG,   \
>> +/** Checks if the context supports a user-facing extension */
>> +#define EXT(name_str,

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

2016-06-13 Thread Nanley Chery 
On Sun, Jun 12, 2016 at 4:23 PM, Ilia Mirkin  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 
> ---
>  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
> -* _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
> -* - ::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, _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 >= \
> +

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

2016-06-13 Thread Eric Engestrom 
On Mon, Jun 13, 2016 at 12:08:24PM +0100, Eric Engestrom wrote:
>   static enum { extension_table_size = 
> ARRAY_SIZE(extension_table_for_size) };

Sorry, I don't know how that `static` snuck in here, but it obviously shouldn't 
be here.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

2016-06-13 Thread Eric Engestrom 
On Sun, Jun 12, 2016 at 07:23:41PM -0400, Ilia Mirkin 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 
> ---

[snip]

> diff --git a/src/mesa/main/extensions.c b/src/mesa/main/extensions.c
> index fa50cb6..3dc9c50 100644
> --- a/src/mesa/main/extensions.c
> +++ b/src/mesa/main/extensions.c
> @@ -49,25 +49,15 @@ static char *extra_extensions = NULL;
>  #define o(x) offsetof(struct gl_extensions, x)
>  
>  
> -/**
> - * \brief Table of supported OpenGL extensions for all API's.
> - */
> -const struct mesa_extension _mesa_extension_table[] = {
> +static bool extension_table_size[] = {
>  #define EXT(name_str, driver_cap, gll_ver, glc_ver, gles_ver, gles2_ver, 
> ) \
> -{ .name = "GL_" #name_str, .offset = o(driver_cap), \
> -  .version = { \
> -[API_OPENGL_COMPAT] = gll_ver, \
> -[API_OPENGL_CORE]   = glc_ver, \
> -[API_OPENGLES]  = gles_ver, \
> -[API_OPENGLES2] = gles2_ver, \
> -   }, \
> -   .year =  \
> -},
> +   0,
> +
>  #include "extensions_table.h"
>  #undef EXT
>  };
>  
> -static bool disabled_extensions[ARRAY_SIZE(_mesa_extension_table)];
> +static bool disabled_extensions[ARRAY_SIZE(extension_table_size)];

/me thinks `extension_table_size` should contain the extension table size...

Maybe this?

static bool extension_table_for_size[] = {
/* ... */
};
static enum { extension_table_size = 
ARRAY_SIZE(extension_table_for_size) };
static bool disabled_extensions[extension_table_size];

>  
>  /**
>   * Given an extension name, lookup up the corresponding member of struct
> @@ -85,7 +75,7 @@ name_to_index(const char* name)
> if (name == 0)
>return -1;
>  
> -   for (i = 0; i < ARRAY_SIZE(_mesa_extension_table); ++i) {
> +   for (i = 0; i < ARRAY_SIZE(extension_table_size); ++i) {

This (and following) is now simply:

for (i = 0; i < extension_table_size; ++i) {

>if (strcmp(name, _mesa_extension_table[i].name) == 0)
>return i;
> }

[snip]

> diff --git a/src/mesa/main/extensions_table.c 
> b/src/mesa/main/extensions_table.c
> new file mode 100644
> index 000..1e37fbc
> --- /dev/null
> +++ b/src/mesa/main/extensions_table.c
> @@ -0,0 +1,51 @@
> +/*
> + * Mesa 3-D graphics library
> + *
> + * Copyright (C) 1999-2008  Brian Paul   All Rights Reserved.
> + * Copyright (C) 2009  VMware, Inc.  All Rights Reserved.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included
> + * in all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
> + * OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF 
> MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR
> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> + * OTHER DEALINGS IN THE SOFTWARE.
> + */
> +
> +#include "main/mtypes.h"
> +#include "main/extensions.h"
> +
> +/**
> + * Given a member \c x of struct gl_extensions, return offset of
> + * \c x in bytes.
> + */
> +#define o(x) offsetof(struct gl_extensions, x)
> +
> +/**
> + * \brief Table of supported OpenGL extensions for all API's.
> + */
> +const struct mesa_extension _mesa_extension_table[] = {
> +#define EXT(name_str, driver_cap, gll_ver, glc_ver, gles_ver, gles2_ver, 
> ) \
> +{ .name = "GL_" #name_str, .offset = o(driver_cap), \
> +  .version = { \
> +[API_OPENGL_COMPAT] = gll_ver, \
> +[API_OPENGL_CORE]   = glc_ver, \
> +[API_OPENGLES]