On Tue, Aug 16, 2016 at 09:21:57PM +0200, Uros Bizjak wrote:
> > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
>
> The patch is basically unreviwable, but we have many checks in the x86
> specific testsuite that would break left and right if something is
> wrong in the area your patch changes. The introduced checks are also
> welcome.
With the patch IX86_BUILTIN_MAX is the same as without the patch (2515), so
in the enum ix86_builtins it is just a permutation of the enumerators.
> > On IRC Richi mentioned also the possibility of using *.def files for this,
> > one possibility would be e.g. to only manually list IX86_BUILTIN_* values
> > for the builtins that aren't mentioned in bdesc* arrays, add
> > config/i386/i386-builtins.def that would contain macros like
> > IX86_BUILTIN_BDESC_FIRST (COMI)
> > IX86_BUILTIN_BDESC (OPTION_MASK_ISA_SSE, sse_comi, __builtin_ia32_comieq,
> > IX86_BUILTIN_COMIEQSS, UNEQ, 0)
> > ...
> > IX86_BUILTIN_BDESC_LAST (COMI)
> > etc. and we'd then include it twice, once inside of enum ix86_builtins
> > to just define the IX86_BUILTIN_* enumerators and again with differently
> > defined macros to define the bdesc_* arrays. Shall I work that as a
> > follow-up, or shall I wait with committing till that is done? Any
> > preferences on the macro names and what info to keep out? E.g. above
> > shows possibility to leave the CODE_FOR_ part out that could be supplied
> > through macro, as all the entries have it. Maybe also __builtin_ia32_ ?
> > IX86_BUILTIN_ ?
>
> The idea is indeed good, but please leave full names in the *.def
> file. We can change them later, if need arises.
Ok, I'll work on it tomorrow.
> The patch is definitely a step in the right direction. The approach
> looks correct, and it opens further cleanups, as you explained above.
>
> So, OK for mainline.
Thanks.
Jakub
