Re: [Mesa-dev] [PATCH 1/9] mesa: Handle common extension checks with more compact code

[Nanley Chery]

On Fri, May 19, 2017 at 04:29:16PM -0700, Ian Romanick wrote:
> On 05/19/2017 10:28 AM, Nanley Chery wrote:
> > On Fri, May 19, 2017 at 06:38:03AM -0700, Ian Romanick wrote:
> >> From: Ian Romanick 
> >>
> >> The previous code handled everything with the general case.  I noticed
> >> that every time I converted an open-coded check to use a
> >> _mesa_has_EXT_foo() function, the text size of the driver increased.
> >>
> >> Almost all extensions only care what the current context API is, and
> >> the version does not matter.  Handle those using more compact checks.
> >>
> >>text   data bss dec hex filename
> >> 7037675 235248   37280 7310203  6f8b7b 32-bit i965_dri.so before
> >> 7034307 235248   37280 7306835  6f7e53 32-bit i965_dri.so after
> >> 6679695 303400   50608 7033703  6b5367 64-bit i965_dri.so before
> >> 6676143 303400   50608 7030151  6b4587 64-bit i965_dri.so after
> > 
> > Hi Ian,
> > 
> > I wrote a patch some time ago that reduces the cost of the extension
> > checks by a lot more with less code. The only thing I think may need
> > addressing is endianness. Would you consider using it instead if I
> > reworked it and sent it out to the list? You can find it here:
> > https://cgit.freedesktop.org/~nchery/mesa/commit/?h=1/ext/optimize=a02d88eba1d3129b27d3b5e6aaa976c3ca20cf79
> 
> I was not able to reproduce that result on current Mesa.  I had a lot
> of trouble believing that more than 18% of our driver binary was
> extension check code.  I also had a sick feeling that may have just
> been the first stage of grief talking... :)  Are you able to reproduce
> your original result?
> 

I'm actually not able to reproduce my results anymore. Also, after
fixing endianness with more bit shifting, I was only able to save ~400B
of .text (using your build flags). I retract my earlier statement.

> I also tried a similar patch to yours that wouldn't have endianness
> problems:
> 
> #define EXT(name_str, driver_cap, gll, glc, es1, es2, ...)
>  \
> static inline bool
>  \
> _mesa_has_##name_str(const struct gl_context *ctx)
>  \
> { 
>  \
>static const uint8_t ver[4] = { (uint8_t)gll, (uint8_t)es1, (uint8_t)es2, 
> (uint8_t)glc }; \
>return ctx->Extensions.driver_cap &&   
>  \
>   (ctx->Extensions.Version >= ver[ctx->API]); 
>  \
> }
> 
> Here's what I got for all four methods:
> 
> 7037675235248   37280 7310203  6f8b7b 32-bit i965_dri.so before
> 7034307235248   37280 7306835  6f7e53 32-bit i965_dri.so after idr
> 7038343235248   37280 7310871  6f8e17 32-bit i965_dri.so after Nanley
> 7036271235248   37280 7308799  6f85ff 32-bit i965_dri.so w/arrays
> 
> 6679695303400   50608 7033703  6b5367 64-bit i965_dri.so before
> 6676143303400   50608 7030151  6b4587 64-bit i965_dri.so after idr
> 6684767303400   50608 7038775  6b6737 64-bit i965_dri.so after Nanley
> 6678567303400   50608 7032575  6b4eff 64-bit i965_dri.so w/arrays
> 
> For reference, I build with the same flags that Fedora 23 (I think?)
> used for release builds:
> 
> -O2 -g -pipe -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector-strong 
> --param=ssp-buffer-size=4 -grecord-gcc-switches
> 
> And either "-m64 -mtune=generic" or "-m32 -march=i686 -mtune=atom
> -fasynchronous-unwind-tables" depending on the platform.
> --enable-debug is not passed to configure.
> 
> Even if I were able to reproduce your original result, there are still
> two cases where your approach may generate more code than is necessary:
> 
> 1. All of the APIs that support the extension use dummy_true.  There
> are many examples of this, but I don't think there are many matching
> users of _mesa_has_XXX_foo().
> 
> 2. All of the APIs support the extension in any version.
> GL_EXT_polygon_offset_clamp is an example.
> 
> I think we can blend the strengths to get something even better.
> 

Agreed.

-Nanley

> I missed that glsl_parser_extras.cpp has its own implementation of the
> has_XXX_foo() functions that take the API and version as explicit
> parameters.  Your patch predates that change.  There's room for some
> modest savings there too.
> 
> I'll send a couple follow-up patches soon.
> 
> > Thanks,
> > Nanley
> 
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 1/9] mesa: Handle common extension checks with more compact code

[Nanley Chery]

On Fri, May 19, 2017 at 05:32:07PM -0700, Ian Romanick wrote:
> On 05/19/2017 05:07 PM, Ilia Mirkin wrote:
> > On Fri, May 19, 2017 at 7:29 PM, Ian Romanick  wrote:
> >> I missed that glsl_parser_extras.cpp has its own implementation of the
> >> has_XXX_foo() functions that take the API and version as explicit
> >> parameters.  Your patch predates that change.  There's room for some
> >> modest savings there too.
> > 
> > The main thing about those functions is that they have to be
> > out-of-line, as they're invoked via function pointer. Each one can be
> > cut down a little using your approach of statically looking at the cap
> > restrictions, but I don't think inlining can work. [I added that code
> > to (a) add "permission" checks to enable extensions in glsl and (b)
> > remove the duplication of adding #defines for all the ext names.]
> 
> Right.  I think there's two approaches to try.  The least intrusive is
> to try some of these tricks in the existing functions.  The more
> intrusive would be to generate the data into the table and modify
> _mesa_glsl_extension::compatible_with_state to use the data instead of
> calling the function.
> 
> I'm leaning towards the latter, but I haven't decided how I'd want to
> generate the data.  The method used by other users of
> main/extensions_table.h won't work because we only want a subset of
> extensions.  Hmm...
> 

I'll have to get back to this next week, but please don't feel blocked
on my comments. This may be the latter trick mentioned above, but if it
isn't, a third option is to initialize an array at context creation time
to describe which extensions are supported. That array is then looked up
in the helpers:
https://cgit.freedesktop.org/~nchery/mesa/log/?h=3/ext/init

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

Re: [Mesa-dev] [PATCH 1/9] mesa: Handle common extension checks with more compact code

[Ian Romanick]

On 05/19/2017 05:07 PM, Ilia Mirkin wrote:
> On Fri, May 19, 2017 at 7:29 PM, Ian Romanick  wrote:
>> I missed that glsl_parser_extras.cpp has its own implementation of the
>> has_XXX_foo() functions that take the API and version as explicit
>> parameters.  Your patch predates that change.  There's room for some
>> modest savings there too.
> 
> The main thing about those functions is that they have to be
> out-of-line, as they're invoked via function pointer. Each one can be
> cut down a little using your approach of statically looking at the cap
> restrictions, but I don't think inlining can work. [I added that code
> to (a) add "permission" checks to enable extensions in glsl and (b)
> remove the duplication of adding #defines for all the ext names.]

Right.  I think there's two approaches to try.  The least intrusive is
to try some of these tricks in the existing functions.  The more
intrusive would be to generate the data into the table and modify
_mesa_glsl_extension::compatible_with_state to use the data instead of
calling the function.

I'm leaning towards the latter, but I haven't decided how I'd want to
generate the data.  The method used by other users of
main/extensions_table.h won't work because we only want a subset of
extensions.  Hmm...

>   -ilia

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

Re: [Mesa-dev] [PATCH 1/9] mesa: Handle common extension checks with more compact code

[Ilia Mirkin]

On Fri, May 19, 2017 at 7:29 PM, Ian Romanick  wrote:
> I missed that glsl_parser_extras.cpp has its own implementation of the
> has_XXX_foo() functions that take the API and version as explicit
> parameters.  Your patch predates that change.  There's room for some
> modest savings there too.

The main thing about those functions is that they have to be
out-of-line, as they're invoked via function pointer. Each one can be
cut down a little using your approach of statically looking at the cap
restrictions, but I don't think inlining can work. [I added that code
to (a) add "permission" checks to enable extensions in glsl and (b)
remove the duplication of adding #defines for all the ext names.]

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

Re: [Mesa-dev] [PATCH 1/9] mesa: Handle common extension checks with more compact code

[Ian Romanick]

On 05/19/2017 10:28 AM, Nanley Chery wrote:
> On Fri, May 19, 2017 at 06:38:03AM -0700, Ian Romanick wrote:
>> From: Ian Romanick 
>>
>> The previous code handled everything with the general case.  I noticed
>> that every time I converted an open-coded check to use a
>> _mesa_has_EXT_foo() function, the text size of the driver increased.
>>
>> Almost all extensions only care what the current context API is, and
>> the version does not matter.  Handle those using more compact checks.
>>
>>text data bss dec hex filename
>> 7037675   235248   37280 7310203  6f8b7b 32-bit i965_dri.so before
>> 7034307   235248   37280 7306835  6f7e53 32-bit i965_dri.so after
>> 6679695   303400   50608 7033703  6b5367 64-bit i965_dri.so before
>> 6676143   303400   50608 7030151  6b4587 64-bit i965_dri.so after
> 
> Hi Ian,
> 
> I wrote a patch some time ago that reduces the cost of the extension
> checks by a lot more with less code. The only thing I think may need
> addressing is endianness. Would you consider using it instead if I
> reworked it and sent it out to the list? You can find it here:
> https://cgit.freedesktop.org/~nchery/mesa/commit/?h=1/ext/optimize=a02d88eba1d3129b27d3b5e6aaa976c3ca20cf79

I was not able to reproduce that result on current Mesa.  I had a lot
of trouble believing that more than 18% of our driver binary was
extension check code.  I also had a sick feeling that may have just
been the first stage of grief talking... :)  Are you able to reproduce
your original result?

I also tried a similar patch to yours that wouldn't have endianness
problems:

#define EXT(name_str, driver_cap, gll, glc, es1, es2, ...) \
static inline bool \
_mesa_has_##name_str(const struct gl_context *ctx) \
{  \
   static const uint8_t ver[4] = { (uint8_t)gll, (uint8_t)es1, (uint8_t)es2, 
(uint8_t)glc }; \
   return ctx->Extensions.driver_cap &&\
  (ctx->Extensions.Version >= ver[ctx->API]);  \
}

Here's what I got for all four methods:

7037675  235248   37280 7310203  6f8b7b 32-bit i965_dri.so before
7034307  235248   37280 7306835  6f7e53 32-bit i965_dri.so after idr
7038343  235248   37280 7310871  6f8e17 32-bit i965_dri.so after Nanley
7036271  235248   37280 7308799  6f85ff 32-bit i965_dri.so w/arrays

6679695  303400   50608 7033703  6b5367 64-bit i965_dri.so before
6676143  303400   50608 7030151  6b4587 64-bit i965_dri.so after idr
6684767  303400   50608 7038775  6b6737 64-bit i965_dri.so after Nanley
6678567  303400   50608 7032575  6b4eff 64-bit i965_dri.so w/arrays

For reference, I build with the same flags that Fedora 23 (I think?)
used for release builds:

-O2 -g -pipe -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector-strong 
--param=ssp-buffer-size=4 -grecord-gcc-switches

And either "-m64 -mtune=generic" or "-m32 -march=i686 -mtune=atom
-fasynchronous-unwind-tables" depending on the platform.
--enable-debug is not passed to configure.

Even if I were able to reproduce your original result, there are still
two cases where your approach may generate more code than is necessary:

1. All of the APIs that support the extension use dummy_true.  There
are many examples of this, but I don't think there are many matching
users of _mesa_has_XXX_foo().

2. All of the APIs support the extension in any version.
GL_EXT_polygon_offset_clamp is an example.

I think we can blend the strengths to get something even better.

I missed that glsl_parser_extras.cpp has its own implementation of the
has_XXX_foo() functions that take the API and version as explicit
parameters.  Your patch predates that change.  There's room for some
modest savings there too.

I'll send a couple follow-up patches soon.

> Thanks,
> Nanley

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

Re: [Mesa-dev] [PATCH 1/9] mesa: Handle common extension checks with more compact code

[Ilia Mirkin]

On Fri, May 19, 2017 at 1:28 PM, Nanley Chery  wrote:
> The only thing I think may need
> addressing is endianness.

FYI, I'm happy to provide BE support, both reviewing code and testing.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 1/9] mesa: Handle common extension checks with more compact code

[Nanley Chery]

On Fri, May 19, 2017 at 06:38:03AM -0700, Ian Romanick wrote:
> From: Ian Romanick 
> 
> The previous code handled everything with the general case.  I noticed
> that every time I converted an open-coded check to use a
> _mesa_has_EXT_foo() function, the text size of the driver increased.
> 
> Almost all extensions only care what the current context API is, and
> the version does not matter.  Handle those using more compact checks.
> 
>text  data bss dec hex filename
> 7037675235248   37280 7310203  6f8b7b 32-bit i965_dri.so before
> 7034307235248   37280 7306835  6f7e53 32-bit i965_dri.so after
> 6679695303400   50608 7033703  6b5367 64-bit i965_dri.so before
> 6676143303400   50608 7030151  6b4587 64-bit i965_dri.so after
> 

Hi Ian,

I wrote a patch some time ago that reduces the cost of the extension
checks by a lot more with less code. The only thing I think may need
addressing is endianness. Would you consider using it instead if I
reworked it and sent it out to the list? You can find it here:
https://cgit.freedesktop.org/~nchery/mesa/commit/?h=1/ext/optimize=a02d88eba1d3129b27d3b5e6aaa976c3ca20cf79

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