| Message ID | 56447A09.4070608@mentor.com |
|---|---|
| State | New |
| Headers | show |
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 don't see the need for giving clone_with_args a new name, just use an overload of clone ()? [ideally C++ would allow us to say that only one overload may be implemented] Thanks, Richard. > Thanks, > - Tom
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. Thanks, - Tom > [ideally C++ would allow us to say that only one overload may be > implemented]
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) > Thanks, > - Tom > > >> [ideally C++ would allow us to say that only one overload may be >> implemented]
Remove first_pass_instance from pass_vrp --- gcc/gen-pass-instances.awk | 32 ++++++++++++++++++++++---------- gcc/pass_manager.h | 2 ++ gcc/passes.c | 20 ++++++++++++++++++++ gcc/passes.def | 4 ++-- gcc/tree-pass.h | 3 ++- gcc/tree-vrp.c | 22 ++++++++++++---------- 6 files changed, 60 insertions(+), 23 deletions(-) diff --git a/gcc/gen-pass-instances.awk b/gcc/gen-pass-instances.awk index cbfaa86..c77bd64 100644 --- a/gcc/gen-pass-instances.awk +++ b/gcc/gen-pass-instances.awk @@ -43,7 +43,7 @@ function handle_line() line = $0; # Find call expression. - call_starts_at = match(line, /NEXT_PASS \(.+\)/); + call_starts_at = match(line, /NEXT_PASS(_WITH_ARG)? \(.+\)/); if (call_starts_at == 0) { print line; @@ -53,23 +53,28 @@ function handle_line() # Length of the call expression. len_of_call = RLENGTH; - len_of_start = length("NEXT_PASS ("); len_of_open = length("("); len_of_close = length(")"); - # Find pass_name argument - len_of_pass_name = len_of_call - (len_of_start + len_of_close); - pass_starts_at = call_starts_at + len_of_start; - pass_name = substr(line, pass_starts_at, len_of_pass_name); - # Find call expression prefix (until and including called function) - prefix_len = pass_starts_at - 1 - len_of_open; - prefix = substr(line, 1, prefix_len); + match(line, /NEXT_PASS(_WITH_ARG)? /) + len_of_call_name = RLENGTH + prefix_len = call_starts_at + len_of_call_name - 1 + prefix = substr(line, 1, prefix_len) # Find call expression postfix postfix_starts_at = call_starts_at + len_of_call; postfix = substr(line, postfix_starts_at); + args_starts_at = prefix_len + 1 + len_of_open; + len_of_args = postfix_starts_at - args_starts_at - len_of_close; + args_str = substr(line, args_starts_at, len_of_args); + split(args_str, args, ","); + + # Find pass_name argument, an optional with_arg argument + pass_name = args[1]; + with_arg = args[2]; + # Set pass_counts if (pass_name in pass_counts) pass_counts[pass_name]++; @@ -79,7 +84,14 @@ function handle_line() pass_num = pass_counts[pass_name]; # Print call expression with extra pass_num argument - printf "%s(%s, %s)%s\n", prefix, pass_name, pass_num, postfix; + printf "%s(", prefix; + printf "%s", pass_name; + printf ", %s", pass_num; + if (with_arg) + { + printf ", %s", with_arg; + } + printf ")%s\n", postfix; } { handle_line() } diff --git a/gcc/pass_manager.h b/gcc/pass_manager.h index 7d539e4..a8199e2 100644 --- a/gcc/pass_manager.h +++ b/gcc/pass_manager.h @@ -120,6 +120,7 @@ private: #define PUSH_INSERT_PASSES_WITHIN(PASS) #define POP_INSERT_PASSES() #define NEXT_PASS(PASS, NUM) opt_pass *PASS ## _ ## NUM +#define NEXT_PASS_WITH_ARG(PASS, NUM, ARG) NEXT_PASS (PASS, NUM) #define TERMINATE_PASS_LIST() #include "pass-instances.def" @@ -128,6 +129,7 @@ private: #undef PUSH_INSERT_PASSES_WITHIN #undef POP_INSERT_PASSES #undef NEXT_PASS +#undef NEXT_PASS_WITH_ARG #undef TERMINATE_PASS_LIST }; // class pass_manager diff --git a/gcc/passes.c b/gcc/passes.c index dd8d00a..0fd365e 100644 --- a/gcc/passes.c +++ b/gcc/passes.c @@ -81,6 +81,12 @@ opt_pass::clone () internal_error ("pass %s does not support cloning", name); } +opt_pass * +opt_pass::clone_with_arg (bool) +{ + internal_error ("pass %s does not support cloning", name); +} + bool opt_pass::gate (function *) { @@ -1572,6 +1578,19 @@ pass_manager::pass_manager (context *ctxt) p = next_pass_1 (p, PASS ## _ ## NUM, PASS ## _1); \ } while (0) +#define NEXT_PASS_WITH_ARG(PASS, NUM, ARG) \ + do { \ + gcc_assert (NULL == PASS ## _ ## NUM); \ + if ((NUM) == 1) \ + PASS ## _1 = make_##PASS (m_ctxt, ARG); \ + else \ + { \ + gcc_assert (PASS ## _1); \ + PASS ## _ ## NUM = PASS ## _1->clone_with_arg (ARG); \ + } \ + p = next_pass_1 (p, PASS ## _ ## NUM, PASS ## _1); \ + } while (0) + #define TERMINATE_PASS_LIST() \ *p = NULL; @@ -1581,6 +1600,7 @@ pass_manager::pass_manager (context *ctxt) #undef PUSH_INSERT_PASSES_WITHIN #undef POP_INSERT_PASSES #undef NEXT_PASS +#undef NEXT_PASS_WITH_ARG #undef TERMINATE_PASS_LIST /* Register the passes with the tree dump code. */ diff --git a/gcc/passes.def b/gcc/passes.def index c0ab6b9..2649d67 100644 --- a/gcc/passes.def +++ b/gcc/passes.def @@ -171,7 +171,7 @@ along with GCC; see the file COPYING3. If not see NEXT_PASS (pass_return_slot); NEXT_PASS (pass_fre); NEXT_PASS (pass_merge_phi); - NEXT_PASS (pass_vrp); + NEXT_PASS_WITH_ARG (pass_vrp, true /* warn_array_bounds_p */); NEXT_PASS (pass_chkp_opt); NEXT_PASS (pass_dce); NEXT_PASS (pass_stdarg); @@ -280,7 +280,7 @@ along with GCC; see the file COPYING3. If not see NEXT_PASS (pass_tracer); NEXT_PASS (pass_dominator); NEXT_PASS (pass_strlen); - NEXT_PASS (pass_vrp); + NEXT_PASS_WITH_ARG (pass_vrp, false /* warn_array_bounds_p */); /* The only const/copy propagation opportunities left after DOM and VRP should be due to degenerate PHI nodes. So rather than run the full propagators, run a specialized pass which diff --git a/gcc/tree-pass.h b/gcc/tree-pass.h index 49e22a9..0e330dd 100644 --- 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); /* This pass and all sub-passes are executed only if the function returns true. The default implementation returns true. */ @@ -439,7 +440,7 @@ extern gimple_opt_pass *make_pass_fre (gcc::context *ctxt); extern gimple_opt_pass *make_pass_check_data_deps (gcc::context *ctxt); extern gimple_opt_pass *make_pass_copy_prop (gcc::context *ctxt); extern gimple_opt_pass *make_pass_isolate_erroneous_paths (gcc::context *ctxt); -extern gimple_opt_pass *make_pass_vrp (gcc::context *ctxt); +extern gimple_opt_pass *make_pass_vrp (gcc::context *ctxt, bool); extern gimple_opt_pass *make_pass_uncprop (gcc::context *ctxt); extern gimple_opt_pass *make_pass_return_slot (gcc::context *ctxt); extern gimple_opt_pass *make_pass_reassoc (gcc::context *ctxt); diff --git a/gcc/tree-vrp.c b/gcc/tree-vrp.c index e2393e4..0ff60fd 100644 --- a/gcc/tree-vrp.c +++ b/gcc/tree-vrp.c @@ -10183,7 +10183,7 @@ finalize_jump_threads (void) /* Traverse all the blocks folding conditionals with known ranges. */ static void -vrp_finalize (void) +vrp_finalize (bool warn_array_bounds_p) { size_t i; @@ -10199,7 +10199,7 @@ vrp_finalize (void) substitute_and_fold (op_with_constant_singleton_value_range, vrp_fold_stmt, false); - if (warn_array_bounds && first_pass_instance) + if (warn_array_bounds && warn_array_bounds_p) check_all_array_refs (); /* We must identify jump threading opportunities before we release @@ -10289,7 +10289,7 @@ vrp_finalize (void) probabilities to aid branch prediction. */ static unsigned int -execute_vrp (void) +execute_vrp (bool warn_array_bounds_p) { int i; edge e; @@ -10313,7 +10313,7 @@ execute_vrp (void) vrp_initialize (); ssa_propagate (vrp_visit_stmt, vrp_visit_phi_node); - vrp_finalize (); + vrp_finalize (warn_array_bounds_p); free_numbers_of_iterations_estimates (cfun); @@ -10385,21 +10385,23 @@ const pass_data pass_data_vrp = class pass_vrp : public gimple_opt_pass { public: - pass_vrp (gcc::context *ctxt) - : gimple_opt_pass (pass_data_vrp, ctxt) + pass_vrp (gcc::context *ctxt, bool warn_array_bounds_p) + : gimple_opt_pass (pass_data_vrp, ctxt), warn_array_bounds_p(warn_array_bounds_p) {} /* opt_pass methods: */ - opt_pass * clone () { return new pass_vrp (m_ctxt); } + opt_pass * clone (bool warn_array_bounds_p) { return new pass_vrp (m_ctxt, warn_array_bounds_p); } virtual bool gate (function *) { return flag_tree_vrp != 0; } - virtual unsigned int execute (function *) { return execute_vrp (); } + virtual unsigned int execute (function *) { return execute_vrp (warn_array_bounds_p); } + private: + bool warn_array_bounds_p; }; // class pass_vrp } // anon namespace gimple_opt_pass * -make_pass_vrp (gcc::context *ctxt) +make_pass_vrp (gcc::context *ctxt, bool warn_array_bounds_p) { - return new pass_vrp (ctxt); + return new pass_vrp (ctxt, warn_array_bounds_p); }