On May 26, 2016 7:06 PM, "Ian Romanick" <<a href="mailto:idr@freedesktop.org">idr@freedesktop.org</a>> 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; > > >