| Message ID | CAFiYyc1vpb_nU_ip9sk69P0P9r2rnBsOOWgJ-j-T9eMk5m3Xqw@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
On Thu, 2015-11-12 at 15:06 +0100, Richard Biener wrote: > On Thu, Nov 12, 2015 at 3:04 PM, Richard Biener > <richard.guenther@gmail.com> wrote: > > On Thu, Nov 12, 2015 at 2:49 PM, Tom de Vries <Tom_deVries@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_deVries@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
On Thu, Nov 12, 2015 at 4:33 PM, David Malcolm <dmalcolm@redhat.com> wrote: > On Thu, 2015-11-12 at 15:06 +0100, Richard Biener wrote: >> On Thu, Nov 12, 2015 at 3:04 PM, Richard Biener >> <richard.guenther@gmail.com> wrote: >> > On Thu, Nov 12, 2015 at 2:49 PM, Tom de Vries <Tom_deVries@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_deVries@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 Yes, and agreed. Richard. > Dave >
--- 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