On Tue, Oct 04, 2016 at 05:48:15PM +0300, Alexander Monakov wrote:
> On Fri, 30 Sep 2016, Jakub Jelinek wrote:
> > This patch allows backends to provide their *-passes.def file with
> > instructions how to ammend passes.def, which then can be inspected in
> > pass-instances.def the script generates.
>
> A few minor comments:
>
> > --- gcc/gen-pass-instances.awk.jj 2016-09-29 22:53:10.264776158 +0200
> > +++ gcc/gen-pass-instances.awk 2016-09-30 13:22:53.745373889 +0200
> > @@ -30,81 +32,201 @@
> > # through:
> > # NEXT_PASS (pass_copy_prop, 8);
> > # (currently there are 8 instances of that pass)
> > +#
> > +# INSERT_PASS_AFTER (pass_copy_prop, 1, pass_stv);
>
> I think it would be nice to mention that a 4th argument can be supplied and if
> so, it will be passed to the pass at run time (although I see the existing
> comment doesn't mention that either).
Yeah, I've followed the earlier comments. The 4th argument is quite rare
thing.
> > +# will insert
> > +# NEXT_PASS (pass_stv, 1);
> > +# after immediately after the NEXT_PASS (pass_copy_prop, 1) line,
>
> Typo in the comment: duplicated 'after'.
Will fix.
> > --- gcc/config/i386/i386-passes.def.jj 2016-09-30 12:54:29.959793738
> > +0200
> > +++ gcc/config/i386/i386-passes.def 2016-09-30 14:01:31.237200032 +0200
> > @@ -0,0 +1,31 @@
> [snip]
> > +
> > +/*
> > + Macros that should be defined used in this file:
> > + INSERT_PASS_AFTER (PASS, INSTANCE, TGT_PASS)
> > + INSERT_PASS_BEFORE (PASS, INSTANCE, TGT_PASS)
> > + REPLACE_PASS (PASS, INSTANCE, TGT_PASS)
> > + */
>
> REPLACE_PASS isn't actually used in this file.
I guess it should be
Macros that can be used in this file:
> > --- gcc/config/i386/i386-protos.h.jj 2016-06-24 12:59:29.000000000
> > +0200
> > +++ gcc/config/i386/i386-protos.h 2016-09-30 14:00:54.759659671 +0200
>
> > @@ -4105,13 +4105,15 @@ class pass_stv : public rtl_opt_pass
> [snip]
> >
> > /* opt_pass methods: */
> > virtual bool gate (function *)
> > {
> > - return TARGET_STV && TARGET_SSE2 && optimize > 1;
> > + return (timode_p ^ (TARGET_64BIT == 0))
> > + && TARGET_STV && TARGET_SSE2 && optimize > 1;
>
> The first line could also be 'timode_p == !!TARGET_64BIT'.
Yeah, that is probably more readable.
> Also I believe this needs parens around the whole expression.
Ok, will do.
Jakub
