[Mesa-dev] [PATCH 4/7] nir/search: Use bitfields to shrink struct sizes

[Jason Ekstrand]

On May 26, 2016 7:06 PM, "Ian Romanick" <idr at freedesktop.org> wrote:
>
> On 05/26/2016 06:30 PM, Jason Ekstrand wrote:
> > This shrinks the .text section of nir_opt_algebraic.o by 30.5 KB:
> >
> >    text     data      bss      dec      hex  filename
> >   48703    64592        0   113295    1ba8f  nir_opt_algebraic.o
> >   17951    64584        0    82535    14267  nir_opt_algebraic.o
> > ---
> >  src/compiler/nir/nir_search.h | 12 ++++++------
> >  1 file changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/src/compiler/nir/nir_search.h
b/src/compiler/nir/nir_search.h
> > index 888a2a3..b97522a 100644
> > --- a/src/compiler/nir/nir_search.h
> > +++ b/src/compiler/nir/nir_search.h
> > @@ -39,23 +39,23 @@ typedef enum {
> >  } nir_search_value_type;
> >
> >  typedef struct {
> > -   nir_search_value_type type;
> > +   uint8_t type; /* enum nir_search_value_type */
>
> Do we lose any checking by having this not be an enum?  Places where the
> compiler would warning about missing cases, etc.  Would telling GCC to
> pack the enum be just as good?  I've gotten similar feedback on similar
> kinds of patches.

The C99 spec states that bit-field elements must be an integer type or
_Bool.  Everything I find indicates that enums aren't allowed.  That said,
GCC does allow them and we did use an enum in a bit-field for
nir_variable.data.mode for a while.  IIRC, it was making MSVC grumpy which
is why we stopped.

I'd personally rather keep it as an enum four the sake of type safety as
you say.  Since this is never included from C++ we may be able to get away
with it but I'm not actually sure that makes a difference.  I added Jose to
the Cc; maybe he can shed sine light on it.

--Jason

> >
> > -   unsigned bit_size;
> > +   uint8_t bit_size;
> >  } nir_search_value;
> >
> >  typedef struct {
> >     nir_search_value value;
> >
> >     /** The variable index;  Must be less than NIR_SEARCH_MAX_VARIABLES
*/
> > -   unsigned variable;
> > +   unsigned variable:7;
> >
> >     /** Indicates that the given variable must be a constant
> >      *
> >      * This is only allowed in search expressions and indicates that the
> >      * given variable is only allowed to match constant values.
> >      */
> > -   bool is_constant;
> > +   bool is_constant:1;
> >
> >     /** Indicates that the given variable must have a certain type
> >      *
> > @@ -67,13 +67,13 @@ typedef struct {
> >      * Note: A variable that is both constant and has a non-void type
will
> >      * never match anything.
> >      */
> > -   nir_alu_type type;
> > +   uint8_t type; /* enum nir_alu_type */
> >  } nir_search_variable;
> >
> >  typedef struct {
> >     nir_search_value value;
> >
> > -   nir_alu_type type;
> > +   uint8_t type; /* enum nir_alu_type */
> >
> >     union {
> >        uint64_t u;
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160526/9d216561/attachment.html>