On Thu, 2015-11-12 at 15:06 +0100, Richard Biener wrote:
> On Thu, Nov 12, 2015 at 3:04 PM, Richard Biener
> <richard.guent...@gmail.com> wrote:
> > On Thu, Nov 12, 2015 at 2:49 PM, Tom de Vries <tom_devr...@mentor.com>
> > wrote:
> >> On 12/11/15 13:26, Richard Biener wrote:
> >>>
> >>> On Thu, Nov 12, 2015 at 12:37 PM, Tom de Vries <tom_devr...@mentor.com>
> >>> wrote:
> >>>>
> >>>> Hi,
> >>>>
> >>>> [ See also related discussion at
> >>>> https://gcc.gnu.org/ml/gcc-patches/2012-07/msg00452.html ]
> >>>>
> >>>> this patch removes the usage of first_pass_instance from pass_vrp.
> >>>>
> >>>> the patch:
> >>>> - limits itself to pass_vrp, but my intention is to remove all
> >>>> usage of first_pass_instance
> >>>> - lacks an update to gdbhooks.py
> >>>>
> >>>> Modifying the pass behaviour depending on the instance number, as
> >>>> first_pass_instance does, break compositionality of the pass list. In
> >>>> other
> >>>> words, adding a pass instance in a pass list may change the behaviour of
> >>>> another instance of that pass in the pass list. Which obviously makes it
> >>>> harder to understand and change the pass list. [ I've filed this issue as
> >>>> PR68247 - Remove pass_first_instance ]
> >>>>
> >>>> The solution is to make the difference in behaviour explicit in the pass
> >>>> list, and no longer change behaviour depending on instance number.
> >>>>
> >>>> One obvious possible fix is to create a duplicate pass with a different
> >>>> name, say 'pass_vrp_warn_array_bounds':
> >>>> ...
> >>>> NEXT_PASS (pass_vrp_warn_array_bounds);
> >>>> ...
> >>>> NEXT_PASS (pass_vrp);
> >>>> ...
> >>>>
> >>>> But, AFAIU that requires us to choose a different dump-file name for each
> >>>> pass. And choosing vrp1 and vrp2 as new dump-file names still means that
> >>>> -fdump-tree-vrp no longer works (which was mentioned as drawback here:
> >>>> https://gcc.gnu.org/ml/gcc-patches/2012-07/msg00453.html ).
> >>>>
> >>>> This patch instead makes pass creation parameterizable. So in the pass
> >>>> list,
> >>>> we use:
> >>>> ...
> >>>> NEXT_PASS_WITH_ARG (pass_vrp, true /* warn_array_bounds_p */);
> >>>> ...
> >>>> NEXT_PASS_WITH_ARG (pass_vrp, false /* warn_array_bounds_p */);
> >>>> ...
> >>>>
> >>>> This approach gives us clarity in the pass list, similar to using a
> >>>> duplicate pass 'pass_vrp_warn_array_bounds'.
> >>>>
> >>>> But it also means -fdump-tree-vrp still works as before.
> >>>>
> >>>> Good idea? Other comments?
> >>>
> >>>
> >>> It's good to get rid of the first_pass_instance hack.
> >>>
> >>> I can't comment on the AWK, leaving that to others. Syntax-wise I'd hoped
> >>> we can just use NEXT_PASS with the extra argument being optional...
> >>
> >>
> >> I suppose I could use NEXT_PASS in the pass list, and expand into
> >> NEXT_PASS_WITH_ARG in pass-instances.def.
> >>
> >> An alternative would be to change the NEXT_PASS macro definitions into
> >> vararg variants. But the last time I submitted something with a vararg
> >> macro
> >> ( https://gcc.gnu.org/ml/gcc-patches/2015-09/msg00794.html ), I got a
> >> question about it (
> >> https://gcc.gnu.org/ml/gcc-patches/2015-09/msg00912.html
> >> ), so I tend to avoid using vararg macros.
> >>
> >>> I don't see the need for giving clone_with_args a new name, just use an
> >>> overload
> >>> of clone ()?
> >>
> >>
> >> That's what I tried initially, but I ran into:
> >> ...
> >> src/gcc/tree-pass.h:85:21: warning: ‘virtual opt_pass* opt_pass::clone()’
> >> was hidden [-Woverloaded-virtual]
> >> virtual opt_pass *clone ();
> >> ^
> >> src/gcc/tree-vrp.c:10393:14: warning: by ‘virtual opt_pass*
> >> {anonymous}::pass_vrp::clone(bool)’ [-Woverloaded-virtual]
> >> opt_pass * clone (bool warn_array_bounds_p) { return new pass_vrp
> >> (m_ctxt, warn_array_bounds_p); }
> >> ...
> >>
> >> Googling the error message gives this discussion: (
> >> http://stackoverflow.com/questions/16505092/confused-about-virtual-overloaded-functions
> >> ), and indeed adding
> >> "using gimple_opt_pass::clone;"
> >> in class pass_vrp makes the warning disappear.
> >>
> >> I'll submit an updated version.
> >
> > Hmm, but actually the above means the pass does not expose the
> > non-argument clone
> > which is good!
> >
> > Or did you forget to add the virtual-with-arg variant to opt_pass?
> > That is, why's it
> > a virtual function in the first place? (clone_with_arg)
>
> That said,
>
> --- a/gcc/tree-pass.h
> +++ b/gcc/tree-pass.h
> @@ -83,6 +83,7 @@ public:
>
> The default implementation prints an error message and aborts. */
> virtual opt_pass *clone ();
> + virtual opt_pass *clone_with_arg (bool);
>
>
> means the arg type is fixed at 'bool' (yeah, mimicing
> first_pass_instance). That
> looks a bit limiting to me, but anyway.
>
> Richard.
>
> >> Thanks,
> >> - Tom
> >>
> >>
> >>> [ideally C++ would allow us to say that only one overload may be
> >>> implemented]
IIRC, the idea of the clone vfunc was to support state management of
passes: to allow all the of the sibling passes within a pass manager to
be able to locate each other, so they can share state if desired,
without sharing state with "cousin" passes in another pass manager (for
a halcyon future in which multiple instances of gcc could be running in
one process in different threads).
I've changed my mind on the merits of this: I think state should be
stored in the IR itself, not in the passes themselves.
I don't think we have any implementations of "clone" that don't simply
call "return new pass_foo ()".
So maybe it makes sense to eliminate clone in favor of being able to
pass arguments to the factory function (and thence to the ctor);
something like:
gimple_opt_pass *
make_pass_vrp (gcc::context *ctxt, bool warn_array_bounds_p)
{
return new pass_vrp (ctxt, warn_array_bounds_p);
}
and then to rewrite passes.c's:
#define NEXT_PASS(PASS, NUM) \
do { \
gcc_assert (NULL == PASS ## _ ## NUM); \
if ((NUM) == 1) \
PASS ## _1 = make_##PASS (m_ctxt); \
else \
{ \
gcc_assert (PASS ## _1); \
PASS ## _ ## NUM = PASS ## _1->clone (); \
} \
p = next_pass_1 (p, PASS ## _ ## NUM, PASS ## _1); \
} while (0)
to something like:
#define NEXT_PASS(PASS, NUM) \
do { \
gcc_assert (NULL == PASS ## _ ## NUM); \
PASS ## _ ## NUM = make_##PASS (m_ctxt);
p = next_pass_1 (p, PASS ## _ ## NUM, PASS ## _1); \
} while (0)
or somesuch, and:
#define NEXT_PASS_WITH_ARG(PASS, NUM, ARG) \
do { \
gcc_assert (NULL == PASS ## _ ## NUM); \
PASS ## _ ## NUM = make_##PASS (m_ctxt, (ARG));
p = next_pass_1 (p, PASS ## _ ## NUM, PASS ## _1); \
} while (0)
Alternatively, if we do want to retain clone, perhaps we could have a
opt_pass::set_arg vfunc?
virtual void set_arg (bool ) { gcc_unreachable (); } /* provide dummy
base class impl, but if you're going to use NEXT_PASS_WITH_ARG, you
really should provide an impl */
with the subclass implementing it like this, to capture it within a
field of the
void pass_vrp::set_arg (bool warn_array_bounds_p)
{
m_warn_array_bounds_p = warn_array_bounds_p;
}
and something like this:
#define NEXT_PASS_WITH_ARG(PASS, NUM, ARG) \
do { \
NEXT_PASS (PASS, NUM); \
PASS ## _ ## NUM->set_arg (ARG); \
} while (0)
or somesuch?
Hope this is constructive
Dave
