On Thu, Nov 12, 2015 at 4:33 PM, David Malcolm <dmalc...@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.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
Yes, and agreed. Richard. > Dave >