Hello,
Any comments?
(patch is here: http://gcc.gnu.org/ml/gcc-patches/2013-07/msg01315.html)
Cheers,
Oleg
On Sat, 2013-07-27 at 14:52 +0200, Oleg Endo wrote:
> Hi,
>
> On Fri, 2013-07-26 at 08:51 +0200, Uros Bizjak wrote:
>
> > BTW: I am not c++ expert, but doesn't c++ offer some sort of
> > abstraction to get rid of
> >
> > + union {
> > + rtx (*argc0) (void);
> > + rtx (*argc1) (rtx);
> > + rtx (*argc2) (rtx, rtx);
> > + rtx (*argc3) (rtx, rtx, rtx);
> > + rtx (*argc4) (rtx, rtx, rtx, rtx);
> > + rtx (*argc5) (rtx, rtx, rtx, rtx, rtx);
> > + rtx (*argc6) (rtx, rtx, rtx, rtx, rtx, rtx);
> > + rtx (*argc7) (rtx, rtx, rtx, rtx, rtx, rtx, rtx);
> > + rtx (*argc8) (rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx);
> > + rtx (*argc9) (rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx);
> > + rtx (*argc10) (rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx);
> > + rtx (*argc11) (rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx);
> > + } genfun;
> >
>
> Variadic templates maybe, but we can't use them since that requires C
> ++11.
>
> Anyway, I think it's better to turn the insn_gen_fn typedef in recog.h
> into a functor. The change is quite transparent to the users, as the
> attached patch shows. There really is no need for things like GEN_FCN?,
> nor do we need to fixup all the backends etc.
>
> I've tested the attached patch on my SH cross setup with 'make all-gcc'
> and it can still produce a valid 'hello world'. Not sure whether it's
> sufficient.
>
> Some notes regarding the patch:
>
> * The whole arg list thing could probably be folded with x-macros or
> something like that, but I don't think it's worth doing it for this
> single case. If we can use C++11 in GCC at some point in time in the
> future, the insn_gen_fn implementation can be folded with variadic
> templates without touching all the code that uses it.
>
> * I had to extend the number of max. args to 16, otherwise the SH
> backend's sync.md code wouldn't compile.
>
> * I don't know whether it's really needed to properly format the code of
> class insn_gen_fn. After reading the first two or three overloads
> (which do fit into 80 columns) one gets the idea and so I guess nobody
> is going to read that stuff completely anyway.
>
> * The class insn_gen_fn is a POD, so it can be passed by value without
> any overhead, just like a normal function pointer. Same goes for the
> function call through the wrapper class.
>
> * Initially I had overloaded constructors in class insn_gen_fn which
> worked and didn't require the cast in genoutput.c. However, it
> introduced static initializer functions and defeated the purpose of the
> generated const struct insn_data_d insn_data[]. This is worked around
> by casting the function pointer to insn_gen_fn::stored_funcptr. (Since
> it's C++ and not C it won't implicitly cast to void*).
>
> * The whole thing will fall apart if the stored pointer to a function
> 'rtx (*)(void)' is different from a stored pointer to e.g. 'rtx
> (*)(rtx)'. But I guess this is not likely to happen.
>
> Cheers,
> Oleg
>
> gcc/ChangeLog:
> * recog.h (rtx (*insn_gen_fn) (rtx, ...)): Replace typedef with
> new class insn_gen_fn.
> * expr.c (move_by_pieces_1, store_by_pieces_2): Replace
> argument rtx (*) (rtx, ...) with insn_gen_fn.
> * genoutput.c (output_insn_data): Cast gen_? function pointers
> to insn_gen_fn::stored_funcptr. Add initializer braces.
