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) > Thanks, > - Tom > > >> [ideally C++ would allow us to say that only one overload may be >> implemented]
