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.
Thanks,
- Tom
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);
+}
+
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 */);
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 */);
/* 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);
/* 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
