On Fri, Jan 3, 2014 at 3:37 PM, Jakub Jelinek <ja...@redhat.com> wrote: > On Fri, Jan 03, 2014 at 03:39:11PM +0000, Joseph S. Myers wrote: >> On Fri, 3 Jan 2014, Jakub Jelinek wrote: >> >> > I've noticed that especially with the AVX512F introduction we use >> > GET_MODE_SIZE (<MODE>mode) quite heavily in the i386 *.md files, and >> > the problem with that is GET_MODE_SIZE isn't a compile time constant, >> > needs to read mode_size array (non-const) at runtime. >> >> It would seem better to me for genmodes to generate appropriate macro / >> inline function definitions that can be used by GET_MODE_SIZE (along the >> lines of (__builtin_constant_p (MODE) && !mode_size_variable (MODE) ? >> get_mode_size_constant (MODE) : (unsigned short) mode_size[MODE]), where >> mode_size_variable and get_mode_size_constant are always_inline functions >> generated by genmodes) - that way all targets are covered automatically, >> without needing such duplication of mode sizes. (Of course such >> optimizations wouldn't apply for a first-stage compiler bootstrapped by a >> compiler not supporting __builtin_constant_p, but lack of optimization in >> the first stage of a bootstrap is not a particular concern.) > > That is certainly doable (as attached), but strangely if the patch (that I've > already committed) is reverted and this one applied, the .text savings are > much smaller. > > Here are .text and .rodata readelf -WS lines from x86_64 (first 4 pairs) and > i686 (last 4 pairs) builds, always vanilla trunk before r206312, that plus > r206312 patch, without r206312 but with attached patch, with both r206312 > and attached patch. So, for .text size the best is both patches, but > for .rodata patches just r206312. I'll try to look at details why this is so > next week. > > [12] .text PROGBITS 00000000004f4b00 0f4b00 1131704 00 > AX 0 0 16 > [14] .rodata PROGBITS 0000000001626240 1226240 4093b4 00 > A 0 0 64 > [12] .text PROGBITS 00000000004f20a0 0f20a0 11156e4 00 > AX 0 0 16 > [14] .rodata PROGBITS 00000000016077c0 12077c0 3fcbb4 00 > A 0 0 64 > [12] .text PROGBITS 00000000004f4c60 0f4c60 112b8b4 00 > AX 0 0 16 > [14] .rodata PROGBITS 0000000001620540 1220540 40b134 00 > A 0 0 64 > [12] .text PROGBITS 00000000004f2200 0f2200 1113eb4 00 > AX 0 0 16 > [14] .rodata PROGBITS 00000000016060c0 12060c0 3fea74 00 > A 0 0 64 > [12] .text PROGBITS 0811d750 0d5750 12b4464 00 AX 0 > 0 16 > [14] .rodata PROGBITS 093d1c00 1389c00 2d8094 00 A 0 > 0 64 > [12] .text PROGBITS 0811b150 0d3150 12996a4 00 AX 0 > 0 16 > [14] .rodata PROGBITS 093b4840 136c840 2d1354 00 A 0 > 0 64 > [12] .text PROGBITS 0811d840 0d5840 12aa1e4 00 AX 0 > 0 16 > [14] .rodata PROGBITS 093c7a40 137fa40 2d8b94 00 A 0 > 0 64 > [12] .text PROGBITS 0811b240 0d3240 1292914 00 AX 0 > 0 16 > [14] .rodata PROGBITS 093adb80 1365b80 2d1f94 00 A 0 > 0 64 > > 2014-01-04 Jakub Jelinek <ja...@redhat.com> > > * genmodes.c (struct mode_data): Add need_bytesize_adj field. > (blank_mdoe): Initialize it. > (emit_mode_size_inline, emit_mode_nunits_inline, > emit_mode_inner_inline): New functions. > (emit_insn_modes_h): Call them and surround their output with > #if GCC_VERSION >= 4001 ... #endif. > * machmode.h (GET_MODE_SIZE, GET_MODE_NUNITS, GET_MODE_INNER): > For GCC_VERSION >= 4001 use mode_*_inline routines instead of > mode_* arrays if the argument is __builtin_constant_p. > * lower-subreg.c (dump_choices): Make sure GET_MODE_SIZE argument > is enum machine_mode. > fortran/ > * trans-types.c (gfc_init_kinds): Make sure GET_MODE_BITSIZE > argument is enum machine_mode.
It turns out Jorn filed a bug about this exact issue (back in 2008). See bug 36109. Thanks, Andrew Pinski > > --- gcc/lower-subreg.c.jj 2013-12-10 18:18:39.077943292 +0100 > +++ gcc/lower-subreg.c 2014-01-03 18:35:00.510418999 +0100 > @@ -1371,7 +1371,7 @@ dump_choices (bool speed_p, const char * > fprintf (dump_file, "Choices when optimizing for %s:\n", description); > > for (i = 0; i < MAX_MACHINE_MODE; i++) > - if (GET_MODE_SIZE (i) > UNITS_PER_WORD) > + if (GET_MODE_SIZE ((enum machine_mode) i) > UNITS_PER_WORD) > fprintf (dump_file, " %s mode %s for copy lowering.\n", > choices[speed_p].move_modes_to_split[i] > ? "Splitting" > --- gcc/fortran/trans-types.c.jj 2013-11-21 22:24:18.790939654 +0100 > +++ gcc/fortran/trans-types.c 2014-01-03 18:35:00.534418997 +0100 > @@ -373,7 +373,7 @@ gfc_init_kinds (void) > /* The middle end doesn't support constants larger than 2*HWI. > Perhaps the target hook shouldn't have accepted these either, > but just to be safe... */ > - bitsize = GET_MODE_BITSIZE (mode); > + bitsize = GET_MODE_BITSIZE ((enum machine_mode) mode); > if (bitsize > 2*HOST_BITS_PER_WIDE_INT) > continue; > > --- gcc/machmode.h.jj 2013-11-29 18:22:15.671594951 +0100 > +++ gcc/machmode.h 2014-01-03 18:47:30.514374282 +0100 > @@ -177,7 +177,13 @@ extern const unsigned char mode_class[NU > /* Get the size in bytes and bits of an object of mode MODE. */ > > extern CONST_MODE_SIZE unsigned char mode_size[NUM_MACHINE_MODES]; > +#if GCC_VERSION >= 4001 > +#define GET_MODE_SIZE(MODE) \ > + ((unsigned short) (__builtin_constant_p (MODE) \ > + ? mode_size_inline (MODE) : mode_size[MODE])) > +#else > #define GET_MODE_SIZE(MODE) ((unsigned short) mode_size[MODE]) > +#endif > #define GET_MODE_BITSIZE(MODE) \ > ((unsigned short) (GET_MODE_SIZE (MODE) * BITS_PER_UNIT)) > > @@ -203,7 +209,13 @@ extern const unsigned HOST_WIDE_INT mode > /* Return the mode of the inner elements in a vector. */ > > extern const unsigned char mode_inner[NUM_MACHINE_MODES]; > +#if GCC_VERSION >= 4001 > +#define GET_MODE_INNER(MODE) \ > + ((enum machine_mode) (__builtin_constant_p (MODE) \ > + ? mode_inner_inline (MODE) : mode_inner[MODE])) > +#else > #define GET_MODE_INNER(MODE) ((enum machine_mode) mode_inner[MODE]) > +#endif > > /* Get the size in bytes or bites of the basic parts of an > object of mode MODE. */ > @@ -224,7 +236,13 @@ extern const unsigned char mode_inner[NU > /* Get the number of units in the object. */ > > extern const unsigned char mode_nunits[NUM_MACHINE_MODES]; > +#if GCC_VERSION >= 4001 > +#define GET_MODE_NUNITS(MODE) \ > + ((unsigned char) (__builtin_constant_p (MODE) \ > + ? mode_nunits_inline (MODE) : mode_nunits[MODE])) > +#else > #define GET_MODE_NUNITS(MODE) mode_nunits[MODE] > +#endif > > /* Get the next wider natural mode (eg, QI -> HI -> SI -> DI -> TI). */ > > --- gcc/genmodes.c.jj 2013-12-16 23:11:04.982989225 +0100 > +++ gcc/genmodes.c 2014-01-03 18:47:16.153375138 +0100 > @@ -72,6 +72,8 @@ struct mode_data > unsigned int counter; /* Rank ordering of modes */ > unsigned int ibit; /* the number of integral bits */ > unsigned int fbit; /* the number of fractional bits */ > + bool need_bytesize_adj; /* true if this mode need dynamic size > + adjustment */ > }; > > static struct mode_data *modes[MAX_MODE_CLASS]; > @@ -82,7 +84,7 @@ static const struct mode_data blank_mode > 0, "<unknown>", MAX_MODE_CLASS, > -1U, -1U, -1U, -1U, > 0, 0, 0, 0, 0, > - "<unknown>", 0, 0, 0, 0 > + "<unknown>", 0, 0, 0, 0, false > }; > > static htab_t modes_by_name; > @@ -904,6 +906,105 @@ emit_max_int (void) > printf ("#define MAX_BITSIZE_MODE_ANY_MODE (%d*BITS_PER_UNIT)\n", mmax); > } > > +/* Emit mode_size_inline routine into insn-modes.h header. */ > +static void > +emit_mode_size_inline (void) > +{ > + int c; > + struct mode_adjust *a; > + struct mode_data *m; > + > + /* Size adjustments must be propagated to all containing modes. */ > + for (a = adj_bytesize; a; a = a->next) > + { > + a->mode->need_bytesize_adj = true; > + for (m = a->mode->contained; m; m = m->next_cont) > + m->need_bytesize_adj = true; > + } > + > + printf ("\ > +#ifdef __cplusplus\n\ > +inline __attribute__((__always_inline__))\n\ > +#else\n\ > +extern __inline__ __attribute__((__always_inline__, __gnu_inline__))\n\ > +#endif\n\ > +unsigned char\n\ > +mode_size_inline (enum machine_mode mode)\n\ > +{\n\ > + extern %sunsigned char mode_size[NUM_MACHINE_MODES];\n\ > + switch (mode)\n\ > + {\n", adj_bytesize ? "" : "const "); > + > + for_all_modes (c, m) > + if (!m->need_bytesize_adj) > + printf (" case %smode: return %u;\n", m->name, m->bytesize); > + > + puts ("\ > + default: return mode_size[mode];\n\ > + }\n\ > +}\n"); > +} > + > +/* Emit mode_nunits_inline routine into insn-modes.h header. */ > +static void > +emit_mode_nunits_inline (void) > +{ > + int c; > + struct mode_data *m; > + > + puts ("\ > +#ifdef __cplusplus\n\ > +inline __attribute__((__always_inline__))\n\ > +#else\n\ > +extern __inline__ __attribute__((__always_inline__, __gnu_inline__))\n\ > +#endif\n\ > +unsigned char\n\ > +mode_nunits_inline (enum machine_mode mode)\n\ > +{\n\ > + extern const unsigned char mode_nunits[NUM_MACHINE_MODES];\n\ > + switch (mode)\n\ > + {"); > + > + for_all_modes (c, m) > + printf (" case %smode: return %u;\n", m->name, m->ncomponents); > + > + puts ("\ > + default: return mode_nunits[mode];\n\ > + }\n\ > +}\n"); > +} > + > +/* Emit mode_inner_inline routine into insn-modes.h header. */ > +static void > +emit_mode_inner_inline (void) > +{ > + int c; > + struct mode_data *m; > + > + puts ("\ > +#ifdef __cplusplus\n\ > +inline __attribute__((__always_inline__))\n\ > +#else\n\ > +extern __inline__ __attribute__((__always_inline__, __gnu_inline__))\n\ > +#endif\n\ > +unsigned char\n\ > +mode_inner_inline (enum machine_mode mode)\n\ > +{\n\ > + extern const unsigned char mode_inner[NUM_MACHINE_MODES];\n\ > + switch (mode)\n\ > + {"); > + > + for_all_modes (c, m) > + printf (" case %smode: return %smode;\n", m->name, > + c != MODE_PARTIAL_INT && m->component > + ? m->component->name : void_mode->name); > + > + puts ("\ > + default: return mode_inner[mode];\n\ > + }\n\ > +}\n"); > +} > + > static void > emit_insn_modes_h (void) > { > @@ -969,6 +1070,12 @@ enum machine_mode\n{"); > printf ("#define CONST_MODE_IBIT%s\n", adj_ibit ? "" : " const"); > printf ("#define CONST_MODE_FBIT%s\n", adj_fbit ? "" : " const"); > emit_max_int (); > + puts ("\n#if GCC_VERSION >= 4001\n"); > + emit_mode_size_inline (); > + emit_mode_nunits_inline (); > + emit_mode_inner_inline (); > + puts ("#endif /* GCC_VERSION >= 4001 */"); > + > puts ("\ > \n\ > #endif /* insn-modes.h */"); > > > Jakub