On Fri, 2015-11-13 at 14:57 +0100, Tom de Vries wrote:
> On 13/11/15 11:35, Richard Biener wrote:
> > 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.
>
> I've implemented the set_arg scenario, though I've renamed it to
> set_pass_param. I've also added a parameter number argument to
> set_pass_param.
>
> Furthermore, I've included the gdbhooks.py update.
>
> OK for trunk if bootstrap and reg-test passes?
>
> Btw, I think
> NEXT_PASS (pass_vrp, false /* warn_array_bounds_p */);
> is now equivalent to
> NEXT_PASS (pass_vrp);
> I'm not sure which one I prefer in passes.def.
We eschew default params in C++ code, so I'd argue that we should also
eschew them in passes.def - I think the first one is far clearer (or to
channel Python, "explicit is better than implicit").
Further comments inline below.
> Thanks,
> - Tom
>
> differences between files attachment
> (0003-Remove-first_pass_instance-from-pass_vrp.patch)
> Remove first_pass_instance from pass_vrp
>
> 2015-11-13 Tom de Vries <t...@codesourcery.com>
>
> * gdbhooks.py (class PassNames): Handle extra arg NEXT_PASS argument.
> * gen-pass-instances.awk (handle_line): Same.
> * pass_manager.h (class pass_manager): Define and undefine
> NEXT_PASS_WITH_ARG.
> * passes.c (opt_pass::set_pass_param): New function.
> (pass_manager::pass_manager): Define and undefine NEXT_PASS_WITH_ARG.
> * passes.def: Add extra arg to NEXT_PASS (pass_vrp).
> * tree-pass.h (gimple_opt::set_pass_param): Declare.
> * tree-vrp.c (vrp_finalize, execute_vrp): Add and handle
> warn_array_bounds_p parameter.
> (pass_vrp::pass_vrp): Initialize warn_array_bounds_p.
> (pass_vrp::set_pass_param): New function.
> (pass_vrp::execute): Add warn_array_bounds_p arg to execute_vrp call.
> (pass_vrp::warn_array_bounds_p): New private member.
>
> ---
> gcc/gdbhooks.py | 2 +-
> gcc/gen-pass-instances.awk | 28 +++++++++++++++++++++++-----
> gcc/pass_manager.h | 2 ++
> gcc/passes.c | 14 ++++++++++++++
> gcc/passes.def | 4 ++--
> gcc/tree-pass.h | 1 +
> gcc/tree-vrp.c | 20 ++++++++++++++------
> 7 files changed, 57 insertions(+), 14 deletions(-)
>
> diff --git a/gcc/gdbhooks.py b/gcc/gdbhooks.py
> index 2b9a94c..f920392 100644
> --- a/gcc/gdbhooks.py
> +++ b/gcc/gdbhooks.py
> @@ -537,7 +537,7 @@ class PassNames:
> self.names = []
> with open(os.path.join(srcdir, 'passes.def')) as f:
> for line in f:
> - m = re.match('\s*NEXT_PASS \((.+)\);', line)
> + m = re.match('\s*NEXT_PASS \(([^,]+).*\);', line)
> if m:
> self.names.append(m.group(1))
>
> diff --git a/gcc/gen-pass-instances.awk b/gcc/gen-pass-instances.awk
> index 9cff429..106a2f6 100644
> --- a/gcc/gen-pass-instances.awk
> +++ b/gcc/gen-pass-instances.awk
> @@ -61,12 +61,14 @@ function handle_line()
> len_of_args = len_of_call - (len_of_start + len_of_close);
> args_start_at = call_starts_at + len_of_start;
> args_str = substr(line, args_start_at, len_of_args);
> + split(args_str, args, ",");
>
> - # Set pass_name argument
> - pass_name = args_str;
> + # Set pass_name argument, an optional with_arg argument
> + pass_name = args[1];
> + with_arg = args[2];
>
> - # Find call expression prefix (until and including called function)
> - len_of_prefix = args_start_at - 1 - len_of_open;
> + # Find call expression prefix
> + len_of_prefix = call_starts_at - 1;
> prefix = substr(line, 1, len_of_prefix);
>
> # Find call expression postfix
> @@ -82,7 +84,23 @@ 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;
> + if (with_arg)
> + {
> + printf "NEXT_PASS_WITH_ARG";
> + }
> + else
> + {
> + printf "NEXT_PASS";
> + }
> + printf " (";
> + 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..e634c5c 100644
> --- a/gcc/passes.c
> +++ b/gcc/passes.c
> @@ -81,6 +81,13 @@ opt_pass::clone ()
> internal_error ("pass %s does not support cloning", name);
> }
>
> +void
> +opt_pass::set_pass_param (unsigned int, bool)
> +{
> + internal_error ("pass %s needs a set_pass_param implementation to handle
> the"
> + " extra argument in NEXT_PASS", name);
> +}
Shouldn't this error message refer to NEXT_PASS_WITH_ARG? (since
set_pass_param only gets called when someone starts using
NEXT_PASS_WITH_ARG in passes.def)
> bool
> opt_pass::gate (function *)
> {
> @@ -1572,6 +1579,12 @@ 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 { \
> + NEXT_PASS (PASS, NUM); \
> + PASS ## _ ## NUM->set_pass_param (0, ARG); \
> + } while (0)
> +
> #define TERMINATE_PASS_LIST() \
> *p = NULL;
>
> @@ -1581,6 +1594,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..3e23edc 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 (pass_vrp, true /* warn_array_bounds_p */);
Shouldn't this be a NEXT_PASS_WITH_ARG?
Does it actually compile? If so, do we need some extra checking to
ensure that we aren't silently dropping args? (my hunch is that
pass-instances.def is silently dropping the arg).
> 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 (pass_vrp, false /* warn_array_bounds_p */);
Likewise.
> /* 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..7b2571f 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 void set_pass_param (unsigned int, bool);
Is the patch missing the implementation of opt_pass::set_pass_param? Do
you see a linker error?
Maybe opt_pass::set_pass_param should have a comment/error explaining to
the developer that they got here because they set NEXT_PASS_WITH_ARG and
didn't implement how the arg gets stored.
> /* This pass and all sub-passes are executed only if the function returns
> true. The default implementation returns true. */
> diff --git a/gcc/tree-vrp.c b/gcc/tree-vrp.c
> index e2393e4..5d085b4 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);
>
> @@ -10386,14 +10386,22 @@ class pass_vrp : public gimple_opt_pass
> {
> public:
> pass_vrp (gcc::context *ctxt)
> - : gimple_opt_pass (pass_data_vrp, ctxt)
> + : gimple_opt_pass (pass_data_vrp, ctxt), warn_array_bounds_p (false)
> {}
>
> /* opt_pass methods: */
> opt_pass * clone () { return new pass_vrp (m_ctxt); }
> + void set_pass_param (unsigned int n, bool param)
> + {
> + gcc_assert (n == 0);
> + warn_array_bounds_p = param;
> + }
> 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
