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]