public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [RFC] Remove first_pass_instance from pass_vrp
@ 2015-11-12 11:38 Tom de Vries
  2015-11-12 12:26 ` Richard Biener
  0 siblings, 1 reply; 23+ messages in thread
From: Tom de Vries @ 2015-11-12 11:38 UTC (permalink / raw)
  To: gcc-patches

[-- Attachment #1: Type: text/plain, Size: 1785 bytes --]

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?

Thanks,
- Tom

[-- Attachment #2: 0001-Remove-first_pass_instance-from-pass_vrp.patch --]
[-- Type: text/x-patch, Size: 8439 bytes --]

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);
 }

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [RFC] Remove first_pass_instance from pass_vrp
  2015-11-12 11:38 [RFC] Remove first_pass_instance from pass_vrp Tom de Vries
@ 2015-11-12 12:26 ` Richard Biener
  2015-11-12 13:49   ` Tom de Vries
  0 siblings, 1 reply; 23+ messages in thread
From: Richard Biener @ 2015-11-12 12:26 UTC (permalink / raw)
  To: Tom de Vries; +Cc: gcc-patches

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

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [RFC] Remove first_pass_instance from pass_vrp
  2015-11-12 12:26 ` Richard Biener
@ 2015-11-12 13:49   ` Tom de Vries
  2015-11-12 14:05     ` Richard Biener
  0 siblings, 1 reply; 23+ messages in thread
From: Tom de Vries @ 2015-11-12 13:49 UTC (permalink / raw)
  To: Richard Biener; +Cc: gcc-patches

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]

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [RFC] Remove first_pass_instance from pass_vrp
  2015-11-12 13:49   ` Tom de Vries
@ 2015-11-12 14:05     ` Richard Biener
  2015-11-12 14:06       ` Richard Biener
  0 siblings, 1 reply; 23+ messages in thread
From: Richard Biener @ 2015-11-12 14:05 UTC (permalink / raw)
  To: Tom de Vries; +Cc: gcc-patches

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]

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [RFC] Remove first_pass_instance from pass_vrp
  2015-11-12 14:05     ` Richard Biener
@ 2015-11-12 14:06       ` Richard Biener
  2015-11-12 15:34         ` David Malcolm
  0 siblings, 1 reply; 23+ messages in thread
From: Richard Biener @ 2015-11-12 14:06 UTC (permalink / raw)
  To: Tom de Vries; +Cc: gcc-patches

On Thu, Nov 12, 2015 at 3:04 PM, Richard Biener
<richard.guenther@gmail.com> wrote:
> 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)

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]

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [RFC] Remove first_pass_instance from pass_vrp
  2015-11-12 14:06       ` Richard Biener
@ 2015-11-12 15:34         ` David Malcolm
  2015-11-13 10:35           ` Richard Biener
  0 siblings, 1 reply; 23+ messages in thread
From: David Malcolm @ 2015-11-12 15:34 UTC (permalink / raw)
  To: Richard Biener; +Cc: Tom de Vries, gcc-patches

On Thu, 2015-11-12 at 15:06 +0100, Richard Biener wrote:
> On Thu, Nov 12, 2015 at 3:04 PM, Richard Biener
> <richard.guenther@gmail.com> wrote:
> > 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)
> 
> 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
Dave

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [RFC] Remove first_pass_instance from pass_vrp
  2015-11-12 15:34         ` David Malcolm
@ 2015-11-13 10:35           ` Richard Biener
  2015-11-13 13:58             ` [PATCH] " Tom de Vries
  0 siblings, 1 reply; 23+ messages in thread
From: Richard Biener @ 2015-11-13 10:35 UTC (permalink / raw)
  To: David Malcolm; +Cc: Tom de Vries, gcc-patches

On Thu, Nov 12, 2015 at 4:33 PM, David Malcolm <dmalcolm@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.guenther@gmail.com> wrote:
>> > 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)
>>
>> 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.

Richard.

> Dave
>

^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH] Remove first_pass_instance from pass_vrp
  2015-11-13 10:35           ` Richard Biener
@ 2015-11-13 13:58             ` Tom de Vries
  2015-11-13 14:12               ` David Malcolm
  2015-11-14  9:19               ` Tom de Vries
  0 siblings, 2 replies; 23+ messages in thread
From: Tom de Vries @ 2015-11-13 13:58 UTC (permalink / raw)
  To: Richard Biener, David Malcolm; +Cc: gcc-patches

[-- Attachment #1: Type: text/plain, Size: 8535 bytes --]

On 13/11/15 11:35, Richard Biener wrote:
> On Thu, Nov 12, 2015 at 4:33 PM, David Malcolm <dmalcolm@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.guenther@gmail.com> wrote:
>>>> 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)
>>>
>>> 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


[-- Attachment #2: 0003-Remove-first_pass_instance-from-pass_vrp.patch --]
[-- Type: text/x-patch, Size: 7920 bytes --]

Remove first_pass_instance from pass_vrp

2015-11-13  Tom de Vries  <tom@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

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] Remove first_pass_instance from pass_vrp
  2015-11-13 13:58             ` [PATCH] " Tom de Vries
@ 2015-11-13 14:12               ` David Malcolm
  2015-11-13 14:19                 ` David Malcolm
  2015-11-13 14:41                 ` Tom de Vries
  2015-11-14  9:19               ` Tom de Vries
  1 sibling, 2 replies; 23+ messages in thread
From: David Malcolm @ 2015-11-13 14:12 UTC (permalink / raw)
  To: Tom de Vries; +Cc: Richard Biener, gcc-patches

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 <dmalcolm@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.guenther@gmail.com> wrote:
> >>>> 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)
> >>>
> >>> 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  <tom@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


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] Remove first_pass_instance from pass_vrp
  2015-11-13 14:12               ` David Malcolm
@ 2015-11-13 14:19                 ` David Malcolm
  2015-11-13 14:41                 ` Tom de Vries
  1 sibling, 0 replies; 23+ messages in thread
From: David Malcolm @ 2015-11-13 14:19 UTC (permalink / raw)
  To: Tom de Vries; +Cc: Richard Biener, gcc-patches

On Fri, 2015-11-13 at 09:12 -0500, David Malcolm wrote:
[...snip...]

> > 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)

[...snip...]

> > 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.

Gah, -ENO_COFFEE; I now see it above (and indeed I commented on it).

Sorry for the noise.
Dave

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] Remove first_pass_instance from pass_vrp
  2015-11-13 14:12               ` David Malcolm
  2015-11-13 14:19                 ` David Malcolm
@ 2015-11-13 14:41                 ` Tom de Vries
  1 sibling, 0 replies; 23+ messages in thread
From: Tom de Vries @ 2015-11-13 14:41 UTC (permalink / raw)
  To: David Malcolm; +Cc: Richard Biener, gcc-patches

On 13/11/15 15:12, David Malcolm wrote:
> On Fri, 2015-11-13 at 14:57 +0100, Tom de Vries wrote:
>> 2015-11-13  Tom de Vries  <tom@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)

We use NEXT_PASS in passes.def. We use gen-pass-instances.awk to 
generate NEXT_PASS_WITH_ARG in pass-instances.def, if NEXT_PASS has an 
extra 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?
>

As explained above, no.

> Does it actually compile?

Yes.

> If so, do we need some extra checking to
> ensure that we aren't silently dropping args?

We could try enforce that pass_vrp can't run unless set_pass_param has 
been called. Not sure how to do this in an elegant way.

> (my hunch is that
> pass-instances.def is silently dropping the arg).

Nope.

>
>>         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.

As explained above, no.

Thanks,
- Tom

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] Remove first_pass_instance from pass_vrp
  2015-11-13 13:58             ` [PATCH] " Tom de Vries
  2015-11-13 14:12               ` David Malcolm
@ 2015-11-14  9:19               ` Tom de Vries
  2015-11-15 10:56                 ` [PATCH, 0/6] Remove first_pass_instance Tom de Vries
  1 sibling, 1 reply; 23+ messages in thread
From: Tom de Vries @ 2015-11-14  9:19 UTC (permalink / raw)
  To: Richard Biener, David Malcolm; +Cc: gcc-patches

On 13/11/15 14:57, Tom de Vries wrote:
> 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?
>

Bootstrap and reg-test on x86_64 succeeded.

OK for trunk?

Thanks,
- Tom

> 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
>
>
> 0003-Remove-first_pass_instance-from-pass_vrp.patch
>
>
> Remove first_pass_instance from pass_vrp
>
> 2015-11-13  Tom de Vries<tom@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

^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH, 0/6] Remove first_pass_instance
  2015-11-14  9:19               ` Tom de Vries
@ 2015-11-15 10:56                 ` Tom de Vries
  2015-11-15 11:00                   ` [PATCH, 2/6] Remove first_pass_instance from pass_reassoc Tom de Vries
                                     ` (5 more replies)
  0 siblings, 6 replies; 23+ messages in thread
From: Tom de Vries @ 2015-11-15 10:56 UTC (permalink / raw)
  To: Richard Biener; +Cc: David Malcolm, gcc-patches

[ was: Re: [PATCH] Remove first_pass_instance from pass_vrp ]

This patch series removes first_pass_instance.

      1	Remove first_pass_instance from pass_vrp
      2	Remove first_pass_instance from pass_reassoc
      3	Remove first_pass_instance from pass_dominator
      4	Remove first_pass_instance from pass_object_sizes
      5	Remove first_pass_instance from pass_ccp
      6	Remove first_pass_instance

Bootstrapped and reg-tested on x86_64.

I will post the individual patches in reply to this email.

[ I won't post the first patch though. It was already posted here: 
https://gcc.gnu.org/ml/gcc-patches/2015-11/msg01701.html . ]

Thanks,
- Tom

^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH, 2/6] Remove first_pass_instance from pass_reassoc
  2015-11-15 10:56                 ` [PATCH, 0/6] Remove first_pass_instance Tom de Vries
@ 2015-11-15 11:00                   ` Tom de Vries
  2015-11-15 11:02                   ` [PATCH, 3/6] Remove first_pass_instance from pass_dominator Tom de Vries
                                     ` (4 subsequent siblings)
  5 siblings, 0 replies; 23+ messages in thread
From: Tom de Vries @ 2015-11-15 11:00 UTC (permalink / raw)
  To: Richard Biener; +Cc: David Malcolm, gcc-patches

[-- Attachment #1: Type: text/plain, Size: 800 bytes --]

On 15/11/15 11:55, Tom de Vries wrote:
> [ was: Re: [PATCH] Remove first_pass_instance from pass_vrp ]
>
> This patch series removes first_pass_instance.
>
>       1    Remove first_pass_instance from pass_vrp
>       2    Remove first_pass_instance from pass_reassoc
>       3    Remove first_pass_instance from pass_dominator
>       4    Remove first_pass_instance from pass_object_sizes
>       5    Remove first_pass_instance from pass_ccp
>       6    Remove first_pass_instance
>
> Bootstrapped and reg-tested on x86_64.
>
> I will post the individual patches in reply to this email.
>
> [ I won't post the first patch though. It was already posted here:
> https://gcc.gnu.org/ml/gcc-patches/2015-11/msg01701.html . ]

this patch removes first_pass_instance from pass_reassoc.

Thanks,
- Tom


[-- Attachment #2: 0002-Remove-first_pass_instance-from-pass_reassoc.patch --]
[-- Type: text/x-patch, Size: 4162 bytes --]

Remove first_pass_instance from pass_reassoc

2015-11-15  Tom de Vries  <tom@codesourcery.com>

	* passes.def: Add arg to pass_reassoc pass instantiation.
	* tree-ssa-reassoc.c (reassoc_insert_powi_p): New static variable.
	(acceptable_pow_call, reassociate_bb): Use reassoc_insert_powi_p instead
	of first_pass_instance.
	(execute_reassoc): Add and handle insert_powi_p parameter.
	(pass_reassoc::insert_powi_p): New private member.
	(pass_reassoc::pass_reassoc): Initialize insert_powi_p.
	(pass_reassoc::set_pass_param): New member function.  Set insert_powi_p.
	(pass_reassoc::execute): Call execute_reassoc with extra arg.

---
 gcc/passes.def         |  4 ++--
 gcc/tree-ssa-reassoc.c | 28 ++++++++++++++++++++++------
 2 files changed, 24 insertions(+), 8 deletions(-)

diff --git a/gcc/passes.def b/gcc/passes.def
index 64c1fa1..78fdf0f 100644
--- a/gcc/passes.def
+++ b/gcc/passes.def
@@ -205,7 +205,7 @@ along with GCC; see the file COPYING3.  If not see
 	 opportunities.  */
       NEXT_PASS (pass_phi_only_cprop);
       NEXT_PASS (pass_dse);
-      NEXT_PASS (pass_reassoc);
+      NEXT_PASS (pass_reassoc, true /* insert_powi_p */);
       NEXT_PASS (pass_dce);
       NEXT_PASS (pass_forwprop);
       NEXT_PASS (pass_phiopt);
@@ -276,7 +276,7 @@ along with GCC; see the file COPYING3.  If not see
       NEXT_PASS (pass_lower_vector_ssa);
       NEXT_PASS (pass_split_paths);
       NEXT_PASS (pass_cse_reciprocals);
-      NEXT_PASS (pass_reassoc);
+      NEXT_PASS (pass_reassoc, false /* insert_powi_p */);
       NEXT_PASS (pass_strength_reduction);
       NEXT_PASS (pass_tracer);
       NEXT_PASS (pass_dominator);
diff --git a/gcc/tree-ssa-reassoc.c b/gcc/tree-ssa-reassoc.c
index a75290c..6b08a59 100644
--- a/gcc/tree-ssa-reassoc.c
+++ b/gcc/tree-ssa-reassoc.c
@@ -172,6 +172,9 @@ along with GCC; see the file COPYING3.  If not see
     destructive update for the associating op, and keep the destructive
     update together for vector sum reduction recognition.  */
 
+/* Enable insertion of __builtin_powi calls during execute_reassoc.  See
+   point 3a in the pass header comment.  */
+static bool reassoc_insert_powi_p;
 
 /* Statistics */
 static struct
@@ -3940,7 +3943,7 @@ acceptable_pow_call (gimple *stmt, tree *base, HOST_WIDE_INT *exponent)
   tree fndecl, arg1;
   REAL_VALUE_TYPE c, cint;
 
-  if (!first_pass_instance
+  if (!reassoc_insert_powi_p
       || !flag_unsafe_math_optimizations
       || !is_gimple_call (stmt)
       || !has_single_use (gimple_call_lhs (stmt)))
@@ -4856,7 +4859,7 @@ reassociate_bb (basic_block bb)
 	      if (rhs_code == MULT_EXPR)
 		attempt_builtin_copysign (&ops);
 
-	      if (first_pass_instance
+	      if (reassoc_insert_powi_p
 		  && rhs_code == MULT_EXPR
 		  && flag_unsafe_math_optimizations)
 		powi_result = attempt_builtin_powi (stmt, &ops);
@@ -5111,11 +5114,14 @@ fini_reassoc (void)
   loop_optimizer_finalize ();
 }
 
-/* Gate and execute functions for Reassociation.  */
+/* Gate and execute functions for Reassociation.  If INSERT_POWI_P, enable
+   insertion of __builtin_powi calls.  */
 
 static unsigned int
-execute_reassoc (void)
+execute_reassoc (bool insert_powi_p)
 {
+  reassoc_insert_powi_p = insert_powi_p;
+
   init_reassoc ();
 
   do_reassoc ();
@@ -5145,14 +5151,24 @@ class pass_reassoc : public gimple_opt_pass
 {
 public:
   pass_reassoc (gcc::context *ctxt)
-    : gimple_opt_pass (pass_data_reassoc, ctxt)
+    : gimple_opt_pass (pass_data_reassoc, ctxt), insert_powi_p (false)
   {}
 
   /* opt_pass methods: */
   opt_pass * clone () { return new pass_reassoc (m_ctxt); }
+  void set_pass_param (unsigned int n, bool param)
+    {
+      gcc_assert (n == 0);
+      insert_powi_p = param;
+    }
   virtual bool gate (function *) { return flag_tree_reassoc != 0; }
-  virtual unsigned int execute (function *) { return execute_reassoc (); }
+  virtual unsigned int execute (function *)
+    { return execute_reassoc (insert_powi_p); }
 
+ private:
+  /* Enable insertion of __builtin_powi calls during execute_reassoc.  See
+     point 3a in the pass header comment.  */
+  bool insert_powi_p;
 }; // class pass_reassoc
 
 } // anon namespace

^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH, 3/6] Remove first_pass_instance from pass_dominator
  2015-11-15 10:56                 ` [PATCH, 0/6] Remove first_pass_instance Tom de Vries
  2015-11-15 11:00                   ` [PATCH, 2/6] Remove first_pass_instance from pass_reassoc Tom de Vries
@ 2015-11-15 11:02                   ` Tom de Vries
  2015-11-15 11:03                   ` [PATCH, 4/6] Remove first_pass_instance from pass_object_sizes Tom de Vries
                                     ` (3 subsequent siblings)
  5 siblings, 0 replies; 23+ messages in thread
From: Tom de Vries @ 2015-11-15 11:02 UTC (permalink / raw)
  To: Richard Biener; +Cc: David Malcolm, gcc-patches

[-- Attachment #1: Type: text/plain, Size: 802 bytes --]

On 15/11/15 11:55, Tom de Vries wrote:
> [ was: Re: [PATCH] Remove first_pass_instance from pass_vrp ]
>
> This patch series removes first_pass_instance.
>
>       1    Remove first_pass_instance from pass_vrp
>       2    Remove first_pass_instance from pass_reassoc
>       3    Remove first_pass_instance from pass_dominator
>       4    Remove first_pass_instance from pass_object_sizes
>       5    Remove first_pass_instance from pass_ccp
>       6    Remove first_pass_instance
>
> Bootstrapped and reg-tested on x86_64.
>
> I will post the individual patches in reply to this email.
>
> [ I won't post the first patch though. It was already posted here:
> https://gcc.gnu.org/ml/gcc-patches/2015-11/msg01701.html . ]

this patch removes first_pass_instance from pass_dominator.

Thanks,
- Tom


[-- Attachment #2: 0003-Remove-first_pass_instance-from-pass_dominator.patch --]
[-- Type: text/x-patch, Size: 4259 bytes --]

Remove first_pass_instance from pass_dominator

2015-11-15  Tom de Vries  <tom@codesourcery.com>

	* passes.def: Add arg to pass_dominator pass instantiation.
	* tree-pass.h (first_pass_instance): Remove pass_dominator-related bit
	of comment.
	* tree-ssa-dom.c (pass_dominator::pass_dominator): Initialize
	may_peel_loop_headers_p.
	(pass_dominator::set_pass_param): New member function.  Set
	may_peel_loop_headers_p.
	(pass_dominator::may_peel_loop_headers_p): New private member.
	(pass_dominator::execute): Use may_peel_loop_headers_p instead of
	first_pass_instance.

---
 gcc/passes.def     |  4 ++--
 gcc/tree-pass.h    |  7 ++-----
 gcc/tree-ssa-dom.c | 16 ++++++++++++++--
 3 files changed, 18 insertions(+), 9 deletions(-)

diff --git a/gcc/passes.def b/gcc/passes.def
index 78fdf0f..d274a95 100644
--- a/gcc/passes.def
+++ b/gcc/passes.def
@@ -190,7 +190,7 @@ along with GCC; see the file COPYING3.  If not see
 	 propagations have already run, but before some more dead code
 	 is removed, and this place fits nicely.  Remember this when
 	 trying to move or duplicate pass_dominator somewhere earlier.  */
-      NEXT_PASS (pass_dominator);
+      NEXT_PASS (pass_dominator, true /* may_peel_loop_headers_p */);
       /* At this point the majority of const/copy propagations
 	 are exposed.  Go ahead and identify paths that should never
 	 be executed in a conforming program and isolate those paths.
@@ -279,7 +279,7 @@ along with GCC; see the file COPYING3.  If not see
       NEXT_PASS (pass_reassoc, false /* insert_powi_p */);
       NEXT_PASS (pass_strength_reduction);
       NEXT_PASS (pass_tracer);
-      NEXT_PASS (pass_dominator);
+      NEXT_PASS (pass_dominator, false /* may_peel_loop_headers_p */);
       NEXT_PASS (pass_strlen);
       NEXT_PASS (pass_vrp, false /* warn_array_bounds_p */);
       /* The only const/copy propagation opportunities left after
diff --git a/gcc/tree-pass.h b/gcc/tree-pass.h
index a672d52..d647e73 100644
--- a/gcc/tree-pass.h
+++ b/gcc/tree-pass.h
@@ -631,11 +631,8 @@ extern bool function_called_by_processed_nodes_p (void);
 
 /* Set to true if the pass is called the first time during compilation of the
    current function.  Note that using this information in the optimization
-   passes is considered not to be clean, and it should be avoided if possible.
-   This flag is currently used to prevent loops from being peeled repeatedly
-   in jump threading; it will be removed once we preserve loop structures
-   throughout the compilation -- we will be able to mark the affected loops
-   directly in jump threading, and avoid peeling them next time.  */
+   passes is considered not to be clean, and it should be avoided if
+   possible.  */
 extern bool first_pass_instance;
 
 /* Declare for plugins.  */
diff --git a/gcc/tree-ssa-dom.c b/gcc/tree-ssa-dom.c
index 5cb2644..aeb726c 100644
--- a/gcc/tree-ssa-dom.c
+++ b/gcc/tree-ssa-dom.c
@@ -536,14 +536,26 @@ class pass_dominator : public gimple_opt_pass
 {
 public:
   pass_dominator (gcc::context *ctxt)
-    : gimple_opt_pass (pass_data_dominator, ctxt)
+    : gimple_opt_pass (pass_data_dominator, ctxt),
+      may_peel_loop_headers_p (false)
   {}
 
   /* opt_pass methods: */
   opt_pass * clone () { return new pass_dominator (m_ctxt); }
+  void set_pass_param (unsigned int n, bool param)
+    {
+      gcc_assert (n == 0);
+      may_peel_loop_headers_p = param;
+    }
   virtual bool gate (function *) { return flag_tree_dom != 0; }
   virtual unsigned int execute (function *);
 
+ private:
+  /* This flag is used to prevent loops from being peeled repeatedly in jump
+     threading; it will be removed once we preserve loop structures throughout
+     the compilation -- we will be able to mark the affected loops directly in
+     jump threading, and avoid peeling them next time.  */
+  bool may_peel_loop_headers_p;
 }; // class pass_dominator
 
 unsigned int
@@ -619,7 +631,7 @@ pass_dominator::execute (function *fun)
   free_all_edge_infos ();
 
   /* Thread jumps, creating duplicate blocks as needed.  */
-  cfg_altered |= thread_through_all_blocks (first_pass_instance);
+  cfg_altered |= thread_through_all_blocks (may_peel_loop_headers_p);
 
   if (cfg_altered)
     free_dominance_info (CDI_DOMINATORS);

^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH, 4/6] Remove first_pass_instance from pass_object_sizes
  2015-11-15 10:56                 ` [PATCH, 0/6] Remove first_pass_instance Tom de Vries
  2015-11-15 11:00                   ` [PATCH, 2/6] Remove first_pass_instance from pass_reassoc Tom de Vries
  2015-11-15 11:02                   ` [PATCH, 3/6] Remove first_pass_instance from pass_dominator Tom de Vries
@ 2015-11-15 11:03                   ` Tom de Vries
  2015-11-15 11:06                   ` [PATCH, 5/6] Remove first_pass_instance from pass_ccp Tom de Vries
                                     ` (2 subsequent siblings)
  5 siblings, 0 replies; 23+ messages in thread
From: Tom de Vries @ 2015-11-15 11:03 UTC (permalink / raw)
  To: Richard Biener; +Cc: David Malcolm, gcc-patches

[-- Attachment #1: Type: text/plain, Size: 807 bytes --]

On 15/11/15 11:55, Tom de Vries wrote:
> [ was: Re: [PATCH] Remove first_pass_instance from pass_vrp ]
>
> This patch series removes first_pass_instance.
>
>       1    Remove first_pass_instance from pass_vrp
>       2    Remove first_pass_instance from pass_reassoc
>       3    Remove first_pass_instance from pass_dominator
>       4    Remove first_pass_instance from pass_object_sizes
>       5    Remove first_pass_instance from pass_ccp
>       6    Remove first_pass_instance
>
> Bootstrapped and reg-tested on x86_64.
>
> I will post the individual patches in reply to this email.
>
> [ I won't post the first patch though. It was already posted here:
> https://gcc.gnu.org/ml/gcc-patches/2015-11/msg01701.html . ]

this patches removes first_pass_instance from pass_object_sizes.

Thanks,
- Tom


[-- Attachment #2: 0004-Remove-first_pass_instance-from-pass_object_sizes.patch --]
[-- Type: text/x-patch, Size: 3073 bytes --]

Remove first_pass_instance from pass_object_sizes

2015-11-15  Tom de Vries  <tom@codesourcery.com>

	* passes.def: Add arg to pass_object_sizes pass instantiation.
	* tree-object-size.c (pass_object_sizes::pass_object_sizes): Initialize
	insert_min_max_p.
	(pass_object_sizes::set_pass_param): New member function.  Set
	insert_min_max_p.
	(pass_object_sizes::insert_min_max_p): New private member.
	(pass_object_sizes::execute): Use insert_min_max_p instead of
	first_pass_instance.

---
 gcc/passes.def         |  4 ++--
 gcc/tree-object-size.c | 14 +++++++++++---
 2 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/gcc/passes.def b/gcc/passes.def
index d274a95..64883a7 100644
--- a/gcc/passes.def
+++ b/gcc/passes.def
@@ -77,7 +77,7 @@ along with GCC; see the file COPYING3.  If not see
       NEXT_PASS (pass_all_early_optimizations);
       PUSH_INSERT_PASSES_WITHIN (pass_all_early_optimizations)
 	  NEXT_PASS (pass_remove_cgraph_callee_edges);
-	  NEXT_PASS (pass_object_sizes);
+	  NEXT_PASS (pass_object_sizes, true /* insert_min_max_p */);
 	  NEXT_PASS (pass_ccp);
 	  /* After CCP we rewrite no longer addressed locals into SSA
 	     form if possible.  */
@@ -164,7 +164,7 @@ along with GCC; see the file COPYING3.  If not see
       NEXT_PASS (pass_backprop);
       NEXT_PASS (pass_phiprop);
       NEXT_PASS (pass_forwprop);
-      NEXT_PASS (pass_object_sizes);
+      NEXT_PASS (pass_object_sizes, false /* insert_min_max_p */);
       /* pass_build_alias is a dummy pass that ensures that we
 	 execute TODO_rebuild_alias at this point.  */
       NEXT_PASS (pass_build_alias);
diff --git a/gcc/tree-object-size.c b/gcc/tree-object-size.c
index fa3625c..459e65d 100644
--- a/gcc/tree-object-size.c
+++ b/gcc/tree-object-size.c
@@ -1217,13 +1217,21 @@ class pass_object_sizes : public gimple_opt_pass
 {
 public:
   pass_object_sizes (gcc::context *ctxt)
-    : gimple_opt_pass (pass_data_object_sizes, ctxt)
+    : gimple_opt_pass (pass_data_object_sizes, ctxt), insert_min_max_p (false)
   {}
 
   /* opt_pass methods: */
   opt_pass * clone () { return new pass_object_sizes (m_ctxt); }
+  void set_pass_param (unsigned int n, bool param)
+    {
+      gcc_assert (n == 0);
+      insert_min_max_p = param;
+    }
   virtual unsigned int execute (function *);
 
+ private:
+  /* Determines whether the pass instance creates MIN/MAX_EXPRs.  */
+  bool insert_min_max_p;
 }; // class pass_object_sizes
 
 /* Dummy valueize function.  */
@@ -1250,12 +1258,12 @@ pass_object_sizes::execute (function *fun)
 
 	  init_object_sizes ();
 
-	  /* In the first pass instance, only attempt to fold
+	  /* If insert_min_max_p, only attempt to fold
 	     __builtin_object_size (x, 1) and __builtin_object_size (x, 3),
 	     and rather than folding the builtin to the constant if any,
 	     create a MIN_EXPR or MAX_EXPR of the __builtin_object_size
 	     call result and the computed constant.  */
-	  if (first_pass_instance)
+	  if (insert_min_max_p)
 	    {
 	      tree ost = gimple_call_arg (call, 1);
 	      if (tree_fits_uhwi_p (ost))

^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH, 5/6] Remove first_pass_instance from pass_ccp
  2015-11-15 10:56                 ` [PATCH, 0/6] Remove first_pass_instance Tom de Vries
                                     ` (2 preceding siblings ...)
  2015-11-15 11:03                   ` [PATCH, 4/6] Remove first_pass_instance from pass_object_sizes Tom de Vries
@ 2015-11-15 11:06                   ` Tom de Vries
  2015-11-15 11:09                   ` [PATCH, 6/6] Remove first_pass_instance Tom de Vries
  2015-11-16 12:09                   ` [PATCH, 0/6] " Bernd Schmidt
  5 siblings, 0 replies; 23+ messages in thread
From: Tom de Vries @ 2015-11-15 11:06 UTC (permalink / raw)
  To: Richard Biener; +Cc: David Malcolm, gcc-patches

[-- Attachment #1: Type: text/plain, Size: 796 bytes --]

On 15/11/15 11:55, Tom de Vries wrote:
> [ was: Re: [PATCH] Remove first_pass_instance from pass_vrp ]
>
> This patch series removes first_pass_instance.
>
>       1    Remove first_pass_instance from pass_vrp
>       2    Remove first_pass_instance from pass_reassoc
>       3    Remove first_pass_instance from pass_dominator
>       4    Remove first_pass_instance from pass_object_sizes
>       5    Remove first_pass_instance from pass_ccp
>       6    Remove first_pass_instance
>
> Bootstrapped and reg-tested on x86_64.
>
> I will post the individual patches in reply to this email.
>
> [ I won't post the first patch though. It was already posted here:
> https://gcc.gnu.org/ml/gcc-patches/2015-11/msg01701.html . ]

this patch removes first_pass_instance from pass_ccp.

Thanks,
- Tom


[-- Attachment #2: 0005-Remove-first_pass_instance-from-pass_ccp.patch --]
[-- Type: text/x-patch, Size: 4858 bytes --]

Remove first_pass_instance from pass_ccp

2015-11-15  Tom de Vries  <tom@codesourcery.com>

	* passes.def: Add arg to pass_ccp pass instantiation.
	* tree-ssa-ccp.c (ccp_finalize): Add param nonzero_p.  Use nonzero_p
	instead of first_pass_instance.
	(do_ssa_ccp): Add and handle param nonzero_p.
	(pass_ccp::pass_ccp): Initialize nonzero_p.
	(pass_ccp::set_pass_param): New member function.  Set nonzero_p.
	(pass_ccp::execute): Call do_ssa_ccp with extra arg.
	(pass_ccp::nonzero_p): New private member.

---
 gcc/passes.def     | 10 ++++++----
 gcc/tree-ssa-ccp.c | 27 +++++++++++++++++----------
 2 files changed, 23 insertions(+), 14 deletions(-)

diff --git a/gcc/passes.def b/gcc/passes.def
index 64883a7..17027786 100644
--- a/gcc/passes.def
+++ b/gcc/passes.def
@@ -78,7 +78,9 @@ along with GCC; see the file COPYING3.  If not see
       PUSH_INSERT_PASSES_WITHIN (pass_all_early_optimizations)
 	  NEXT_PASS (pass_remove_cgraph_callee_edges);
 	  NEXT_PASS (pass_object_sizes, true /* insert_min_max_p */);
-	  NEXT_PASS (pass_ccp);
+	  /* Don't record nonzero bits before IPA to avoid
+	     using too much memory.  */
+	  NEXT_PASS (pass_ccp, false /* nonzero_p */);
 	  /* After CCP we rewrite no longer addressed locals into SSA
 	     form if possible.  */
 	  NEXT_PASS (pass_forwprop);
@@ -157,7 +159,7 @@ along with GCC; see the file COPYING3.  If not see
       /* Initial scalar cleanups before alias computation.
 	 They ensure memory accesses are not indirect wherever possible.  */
       NEXT_PASS (pass_strip_predict_hints);
-      NEXT_PASS (pass_ccp);
+      NEXT_PASS (pass_ccp, true /* nonzero_p */);
       /* After CCP we rewrite no longer addressed locals into SSA
 	 form if possible.  */
       NEXT_PASS (pass_complete_unrolli);
@@ -209,7 +211,7 @@ along with GCC; see the file COPYING3.  If not see
       NEXT_PASS (pass_dce);
       NEXT_PASS (pass_forwprop);
       NEXT_PASS (pass_phiopt);
-      NEXT_PASS (pass_ccp);
+      NEXT_PASS (pass_ccp, true /* nonzero_p */);
       /* After CCP we rewrite no longer addressed locals into SSA
 	 form if possible.  */
       NEXT_PASS (pass_cse_sincos);
@@ -319,7 +321,7 @@ along with GCC; see the file COPYING3.  If not see
       NEXT_PASS (pass_lower_complex);
       NEXT_PASS (pass_lower_vector_ssa);
       /* Perform simple scalar cleanup which is constant/copy propagation.  */
-      NEXT_PASS (pass_ccp);
+      NEXT_PASS (pass_ccp, true /* nonzero_p */);
       NEXT_PASS (pass_object_sizes);
       /* Fold remaining builtins.  */
       NEXT_PASS (pass_fold_builtins);
diff --git a/gcc/tree-ssa-ccp.c b/gcc/tree-ssa-ccp.c
index d09fab1..b845a7b 100644
--- a/gcc/tree-ssa-ccp.c
+++ b/gcc/tree-ssa-ccp.c
@@ -886,12 +886,12 @@ do_dbg_cnt (void)
 
 
 /* Do final substitution of propagated values, cleanup the flowgraph and
-   free allocated storage.
+   free allocated storage.  If NONZERO_P, record nonzero bits.
 
    Return TRUE when something was optimized.  */
 
 static bool
-ccp_finalize (void)
+ccp_finalize (bool nonzero_p)
 {
   bool something_changed;
   unsigned i;
@@ -910,9 +910,7 @@ ccp_finalize (void)
       if (!name
 	  || (!POINTER_TYPE_P (TREE_TYPE (name))
 	      && (!INTEGRAL_TYPE_P (TREE_TYPE (name))
-		  /* Don't record nonzero bits before IPA to avoid
-		     using too much memory.  */
-		  || first_pass_instance)))
+		  || !nonzero_p)))
 	continue;
 
       val = get_value (name);
@@ -2394,16 +2392,17 @@ ccp_visit_stmt (gimple *stmt, edge *taken_edge_p, tree *output_p)
 }
 
 
-/* Main entry point for SSA Conditional Constant Propagation.  */
+/* Main entry point for SSA Conditional Constant Propagation.  If NONZERO_P,
+   record nonzero bits.  */
 
 static unsigned int
-do_ssa_ccp (void)
+do_ssa_ccp (bool nonzero_p)
 {
   unsigned int todo = 0;
   calculate_dominance_info (CDI_DOMINATORS);
   ccp_initialize ();
   ssa_propagate (ccp_visit_stmt, ccp_visit_phi_node);
-  if (ccp_finalize ())
+  if (ccp_finalize (nonzero_p))
     todo = (TODO_cleanup_cfg | TODO_update_ssa);
   free_dominance_info (CDI_DOMINATORS);
   return todo;
@@ -2429,14 +2428,22 @@ class pass_ccp : public gimple_opt_pass
 {
 public:
   pass_ccp (gcc::context *ctxt)
-    : gimple_opt_pass (pass_data_ccp, ctxt)
+    : gimple_opt_pass (pass_data_ccp, ctxt), nonzero_p (false)
   {}
 
   /* opt_pass methods: */
   opt_pass * clone () { return new pass_ccp (m_ctxt); }
+  void set_pass_param (unsigned int n, bool param)
+    {
+      gcc_assert (n == 0);
+      nonzero_p = param;
+    }
   virtual bool gate (function *) { return flag_tree_ccp != 0; }
-  virtual unsigned int execute (function *) { return do_ssa_ccp (); }
+  virtual unsigned int execute (function *) { return do_ssa_ccp (nonzero_p); }
 
+ private:
+  /* Determines whether the pass instance records nonzero bits.  */
+  bool nonzero_p;
 }; // class pass_ccp
 
 } // anon namespace

^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH, 6/6] Remove first_pass_instance
  2015-11-15 10:56                 ` [PATCH, 0/6] Remove first_pass_instance Tom de Vries
                                     ` (3 preceding siblings ...)
  2015-11-15 11:06                   ` [PATCH, 5/6] Remove first_pass_instance from pass_ccp Tom de Vries
@ 2015-11-15 11:09                   ` Tom de Vries
  2015-11-15 15:24                     ` David Malcolm
  2015-11-16 12:09                   ` [PATCH, 0/6] " Bernd Schmidt
  5 siblings, 1 reply; 23+ messages in thread
From: Tom de Vries @ 2015-11-15 11:09 UTC (permalink / raw)
  To: Richard Biener; +Cc: David Malcolm, gcc-patches

[-- Attachment #1: Type: text/plain, Size: 795 bytes --]

On 15/11/15 11:55, Tom de Vries wrote:
> [ was: Re: [PATCH] Remove first_pass_instance from pass_vrp ]
>
> This patch series removes first_pass_instance.
>
>       1    Remove first_pass_instance from pass_vrp
>       2    Remove first_pass_instance from pass_reassoc
>       3    Remove first_pass_instance from pass_dominator
>       4    Remove first_pass_instance from pass_object_sizes
>       5    Remove first_pass_instance from pass_ccp
>       6    Remove first_pass_instance
>
> Bootstrapped and reg-tested on x86_64.
>
> I will post the individual patches in reply to this email.
>
> [ I won't post the first patch though. It was already posted here:
> https://gcc.gnu.org/ml/gcc-patches/2015-11/msg01701.html . ]

this patch removes the variable first_pass_instance.

Thanks,
- Tom


[-- Attachment #2: 0006-Remove-first_pass_instance.patch --]
[-- Type: text/x-patch, Size: 1624 bytes --]

Remove first_pass_instance

2015-11-15  Tom de Vries  <tom@codesourcery.com>

	* passes.c (first_pass_instance): Remove variable.
	(execute_todo): Remove setting of first_pass_instance.
	* tree-pass.h (first_pass_instance): Remove declaration.

---
 gcc/passes.c    | 4 ----
 gcc/tree-pass.h | 6 ------
 2 files changed, 10 deletions(-)

diff --git a/gcc/passes.c b/gcc/passes.c
index e634c5c..0e23dcb 100644
--- a/gcc/passes.c
+++ b/gcc/passes.c
@@ -151,7 +151,6 @@ debug_pass (void)
 
 /* Global variables used to communicate with passes.  */
 bool in_gimple_form;
-bool first_pass_instance;
 
 
 /* This is called from various places for FUNCTION_DECL, VAR_DECL,
@@ -2005,9 +2004,6 @@ execute_todo (unsigned int flags)
 
   timevar_push (TV_TODO);
 
-  /* Inform the pass whether it is the first time it is run.  */
-  first_pass_instance = (flags & TODO_mark_first_instance) != 0;
-
   statistics_fini_pass ();
 
   if (flags)
diff --git a/gcc/tree-pass.h b/gcc/tree-pass.h
index d647e73..dcd2d5e 100644
--- a/gcc/tree-pass.h
+++ b/gcc/tree-pass.h
@@ -629,12 +629,6 @@ extern void ipa_read_optimization_summaries (void);
 extern void register_one_dump_file (opt_pass *);
 extern bool function_called_by_processed_nodes_p (void);
 
-/* Set to true if the pass is called the first time during compilation of the
-   current function.  Note that using this information in the optimization
-   passes is considered not to be clean, and it should be avoided if
-   possible.  */
-extern bool first_pass_instance;
-
 /* Declare for plugins.  */
 extern void do_per_function_toporder (void (*) (function *, void *), void *);
 

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH, 6/6] Remove first_pass_instance
  2015-11-15 11:09                   ` [PATCH, 6/6] Remove first_pass_instance Tom de Vries
@ 2015-11-15 15:24                     ` David Malcolm
  2015-11-15 16:23                       ` Tom de Vries
  0 siblings, 1 reply; 23+ messages in thread
From: David Malcolm @ 2015-11-15 15:24 UTC (permalink / raw)
  To: Tom de Vries; +Cc: Richard Biener, gcc-patches

On Sun, 2015-11-15 at 12:08 +0100, Tom de Vries wrote:
> On 15/11/15 11:55, Tom de Vries wrote:
> > [ was: Re: [PATCH] Remove first_pass_instance from pass_vrp ]
> >
> > This patch series removes first_pass_instance.
> >
> >       1    Remove first_pass_instance from pass_vrp
> >       2    Remove first_pass_instance from pass_reassoc
> >       3    Remove first_pass_instance from pass_dominator
> >       4    Remove first_pass_instance from pass_object_sizes
> >       5    Remove first_pass_instance from pass_ccp
> >       6    Remove first_pass_instance
> >
> > Bootstrapped and reg-tested on x86_64.
> >
> > I will post the individual patches in reply to this email.
> >
> > [ I won't post the first patch though. It was already posted here:
> > https://gcc.gnu.org/ml/gcc-patches/2015-11/msg01701.html . ]
> 
> this patch removes the variable first_pass_instance.

Can we also get rid of TODO_mark_first_instance, or would that be a
followup?


> 
> Thanks,
> - Tom
> 
> differences between files attachment
> (0006-Remove-first_pass_instance.patch)
> Remove first_pass_instance
> 
> 2015-11-15  Tom de Vries  <tom@codesourcery.com>
> 
> 	* passes.c (first_pass_instance): Remove variable.
> 	(execute_todo): Remove setting of first_pass_instance.
> 	* tree-pass.h (first_pass_instance): Remove declaration.
> 
> ---
>  gcc/passes.c    | 4 ----
>  gcc/tree-pass.h | 6 ------
>  2 files changed, 10 deletions(-)
> 
> diff --git a/gcc/passes.c b/gcc/passes.c
> index e634c5c..0e23dcb 100644
> --- a/gcc/passes.c
> +++ b/gcc/passes.c
> @@ -151,7 +151,6 @@ debug_pass (void)
>  
>  /* Global variables used to communicate with passes.  */
>  bool in_gimple_form;
> -bool first_pass_instance;
>  
> 
>  /* This is called from various places for FUNCTION_DECL, VAR_DECL,
> @@ -2005,9 +2004,6 @@ execute_todo (unsigned int flags)
>  
>    timevar_push (TV_TODO);
>  
> -  /* Inform the pass whether it is the first time it is run.  */
> -  first_pass_instance = (flags & TODO_mark_first_instance) != 0;
> -
>    statistics_fini_pass ();
>  
>    if (flags)
> diff --git a/gcc/tree-pass.h b/gcc/tree-pass.h
> index d647e73..dcd2d5e 100644
> --- a/gcc/tree-pass.h
> +++ b/gcc/tree-pass.h
> @@ -629,12 +629,6 @@ extern void ipa_read_optimization_summaries (void);
>  extern void register_one_dump_file (opt_pass *);
>  extern bool function_called_by_processed_nodes_p (void);
>  
> -/* Set to true if the pass is called the first time during compilation of the
> -   current function.  Note that using this information in the optimization
> -   passes is considered not to be clean, and it should be avoided if
> -   possible.  */
> -extern bool first_pass_instance;
> -
>  /* Declare for plugins.  */
>  extern void do_per_function_toporder (void (*) (function *, void *), void *);
>  


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH, 6/6] Remove first_pass_instance
  2015-11-15 15:24                     ` David Malcolm
@ 2015-11-15 16:23                       ` Tom de Vries
  2015-11-15 22:55                         ` Tom de Vries
  0 siblings, 1 reply; 23+ messages in thread
From: Tom de Vries @ 2015-11-15 16:23 UTC (permalink / raw)
  To: David Malcolm; +Cc: Richard Biener, gcc-patches

On 15/11/15 16:24, David Malcolm wrote:
> On Sun, 2015-11-15 at 12:08 +0100, Tom de Vries wrote:
>> On 15/11/15 11:55, Tom de Vries wrote:
>>> [ was: Re: [PATCH] Remove first_pass_instance from pass_vrp ]
>>>
>>> This patch series removes first_pass_instance.
>>>
>>>        1    Remove first_pass_instance from pass_vrp
>>>        2    Remove first_pass_instance from pass_reassoc
>>>        3    Remove first_pass_instance from pass_dominator
>>>        4    Remove first_pass_instance from pass_object_sizes
>>>        5    Remove first_pass_instance from pass_ccp
>>>        6    Remove first_pass_instance
>>>
>>> Bootstrapped and reg-tested on x86_64.
>>>
>>> I will post the individual patches in reply to this email.
>>>
>>> [ I won't post the first patch though. It was already posted here:
>>> https://gcc.gnu.org/ml/gcc-patches/2015-11/msg01701.html . ]
>>
>> this patch removes the variable first_pass_instance.
>
> Can we also get rid of TODO_mark_first_instance, or would that be a
> followup?

TODO_mark_first_instance is used in position_pass, which AFAIU is used 
in the plugin infrastructure. I'm not familiar with the plugin 
infrastructure and concepts, so I can't say anything sensible about 
whether we can get rid of the flag.

Thanks,
- Tom

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH, 6/6] Remove first_pass_instance
  2015-11-15 16:23                       ` Tom de Vries
@ 2015-11-15 22:55                         ` Tom de Vries
  0 siblings, 0 replies; 23+ messages in thread
From: Tom de Vries @ 2015-11-15 22:55 UTC (permalink / raw)
  To: David Malcolm; +Cc: Richard Biener, gcc-patches, Le-Chun Wu

[ Adding CC Le-Chun Wu ]

On 15/11/15 17:22, Tom de Vries wrote:
> On 15/11/15 16:24, David Malcolm wrote:
>> On Sun, 2015-11-15 at 12:08 +0100, Tom de Vries wrote:
>>> On 15/11/15 11:55, Tom de Vries wrote:
>>>> [ was: Re: [PATCH] Remove first_pass_instance from pass_vrp ]
>>>>
>>>> This patch series removes first_pass_instance.
>>>>
>>>>        1    Remove first_pass_instance from pass_vrp
>>>>        2    Remove first_pass_instance from pass_reassoc
>>>>        3    Remove first_pass_instance from pass_dominator
>>>>        4    Remove first_pass_instance from pass_object_sizes
>>>>        5    Remove first_pass_instance from pass_ccp
>>>>        6    Remove first_pass_instance
>>>>
>>>> Bootstrapped and reg-tested on x86_64.
>>>>
>>>> I will post the individual patches in reply to this email.
>>>>
>>>> [ I won't post the first patch though. It was already posted here:
>>>> https://gcc.gnu.org/ml/gcc-patches/2015-11/msg01701.html . ]
>>>
>>> this patch removes the variable first_pass_instance.
>>
>> Can we also get rid of TODO_mark_first_instance, or would that be a
>> followup?
>
> TODO_mark_first_instance is used in position_pass, which AFAIU is used
> in the plugin infrastructure. I'm not familiar with the plugin
> infrastructure and concepts, so I can't say anything sensible about
> whether we can get rid of the flag.
>
> Thanks,
> - Tom

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH, 0/6] Remove first_pass_instance
  2015-11-15 10:56                 ` [PATCH, 0/6] Remove first_pass_instance Tom de Vries
                                     ` (4 preceding siblings ...)
  2015-11-15 11:09                   ` [PATCH, 6/6] Remove first_pass_instance Tom de Vries
@ 2015-11-16 12:09                   ` Bernd Schmidt
  2015-11-16 13:05                     ` Tom de Vries
  5 siblings, 1 reply; 23+ messages in thread
From: Bernd Schmidt @ 2015-11-16 12:09 UTC (permalink / raw)
  To: Tom de Vries, Richard Biener; +Cc: David Malcolm, gcc-patches

On 11/15/2015 11:55 AM, Tom de Vries wrote:
> [ was: Re: [PATCH] Remove first_pass_instance from pass_vrp ]
>
> This patch series removes first_pass_instance.
>
>       1    Remove first_pass_instance from pass_vrp
>       2    Remove first_pass_instance from pass_reassoc
>       3    Remove first_pass_instance from pass_dominator
>       4    Remove first_pass_instance from pass_object_sizes
>       5    Remove first_pass_instance from pass_ccp
>       6    Remove first_pass_instance

In 5/6 please retain the comment about memory usage. Otherwise all ok.


Bernd

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH, 0/6] Remove first_pass_instance
  2015-11-16 12:09                   ` [PATCH, 0/6] " Bernd Schmidt
@ 2015-11-16 13:05                     ` Tom de Vries
  0 siblings, 0 replies; 23+ messages in thread
From: Tom de Vries @ 2015-11-16 13:05 UTC (permalink / raw)
  To: Bernd Schmidt, Richard Biener; +Cc: David Malcolm, gcc-patches

[-- Attachment #1: Type: text/plain, Size: 738 bytes --]

On 16/11/15 13:09, Bernd Schmidt wrote:
> On 11/15/2015 11:55 AM, Tom de Vries wrote:
>> [ was: Re: [PATCH] Remove first_pass_instance from pass_vrp ]
>>
>> This patch series removes first_pass_instance.
>>
>>       1    Remove first_pass_instance from pass_vrp
>>       2    Remove first_pass_instance from pass_reassoc
>>       3    Remove first_pass_instance from pass_dominator
>>       4    Remove first_pass_instance from pass_object_sizes
>>       5    Remove first_pass_instance from pass_ccp
>>       6    Remove first_pass_instance
>
> In 5/6 please retain the comment about memory usage.

[ FWIW, I moved that comment to passes.def, since I thought it was more 
appropriate there. ]

Done, comitted as attached.

Thanks,
- Tom

[-- Attachment #2: 0005-Remove-first_pass_instance-from-pass_ccp.patch --]
[-- Type: text/x-patch, Size: 4796 bytes --]

Remove first_pass_instance from pass_ccp

2015-11-15  Tom de Vries  <tom@codesourcery.com>

	* passes.def: Add arg to pass_ccp pass instantiation.
	* tree-ssa-ccp.c (ccp_finalize): Add param nonzero_p.  Use nonzero_p
	instead of first_pass_instance.
	(do_ssa_ccp): Add and handle param nonzero_p.
	(pass_ccp::pass_ccp): Initialize nonzero_p.
	(pass_ccp::set_pass_param): New member function.  Set nonzero_p.
	(pass_ccp::execute): Call do_ssa_ccp with extra arg.
	(pass_ccp::nonzero_p): New private member.

---
 gcc/passes.def     | 10 ++++++----
 gcc/tree-ssa-ccp.c | 25 +++++++++++++++++--------
 2 files changed, 23 insertions(+), 12 deletions(-)

diff --git a/gcc/passes.def b/gcc/passes.def
index 64883a7..17027786 100644
--- a/gcc/passes.def
+++ b/gcc/passes.def
@@ -78,7 +78,9 @@ along with GCC; see the file COPYING3.  If not see
       PUSH_INSERT_PASSES_WITHIN (pass_all_early_optimizations)
 	  NEXT_PASS (pass_remove_cgraph_callee_edges);
 	  NEXT_PASS (pass_object_sizes, true /* insert_min_max_p */);
-	  NEXT_PASS (pass_ccp);
+	  /* Don't record nonzero bits before IPA to avoid
+	     using too much memory.  */
+	  NEXT_PASS (pass_ccp, false /* nonzero_p */);
 	  /* After CCP we rewrite no longer addressed locals into SSA
 	     form if possible.  */
 	  NEXT_PASS (pass_forwprop);
@@ -157,7 +159,7 @@ along with GCC; see the file COPYING3.  If not see
       /* Initial scalar cleanups before alias computation.
 	 They ensure memory accesses are not indirect wherever possible.  */
       NEXT_PASS (pass_strip_predict_hints);
-      NEXT_PASS (pass_ccp);
+      NEXT_PASS (pass_ccp, true /* nonzero_p */);
       /* After CCP we rewrite no longer addressed locals into SSA
 	 form if possible.  */
       NEXT_PASS (pass_complete_unrolli);
@@ -209,7 +211,7 @@ along with GCC; see the file COPYING3.  If not see
       NEXT_PASS (pass_dce);
       NEXT_PASS (pass_forwprop);
       NEXT_PASS (pass_phiopt);
-      NEXT_PASS (pass_ccp);
+      NEXT_PASS (pass_ccp, true /* nonzero_p */);
       /* After CCP we rewrite no longer addressed locals into SSA
 	 form if possible.  */
       NEXT_PASS (pass_cse_sincos);
@@ -319,7 +321,7 @@ along with GCC; see the file COPYING3.  If not see
       NEXT_PASS (pass_lower_complex);
       NEXT_PASS (pass_lower_vector_ssa);
       /* Perform simple scalar cleanup which is constant/copy propagation.  */
-      NEXT_PASS (pass_ccp);
+      NEXT_PASS (pass_ccp, true /* nonzero_p */);
       NEXT_PASS (pass_object_sizes);
       /* Fold remaining builtins.  */
       NEXT_PASS (pass_fold_builtins);
diff --git a/gcc/tree-ssa-ccp.c b/gcc/tree-ssa-ccp.c
index d09fab1..7b6b451 100644
--- a/gcc/tree-ssa-ccp.c
+++ b/gcc/tree-ssa-ccp.c
@@ -886,12 +886,12 @@ do_dbg_cnt (void)
 
 
 /* Do final substitution of propagated values, cleanup the flowgraph and
-   free allocated storage.
+   free allocated storage.  If NONZERO_P, record nonzero bits.
 
    Return TRUE when something was optimized.  */
 
 static bool
-ccp_finalize (void)
+ccp_finalize (bool nonzero_p)
 {
   bool something_changed;
   unsigned i;
@@ -912,7 +912,7 @@ ccp_finalize (void)
 	      && (!INTEGRAL_TYPE_P (TREE_TYPE (name))
 		  /* Don't record nonzero bits before IPA to avoid
 		     using too much memory.  */
-		  || first_pass_instance)))
+		  || !nonzero_p)))
 	continue;
 
       val = get_value (name);
@@ -2394,16 +2394,17 @@ ccp_visit_stmt (gimple *stmt, edge *taken_edge_p, tree *output_p)
 }
 
 
-/* Main entry point for SSA Conditional Constant Propagation.  */
+/* Main entry point for SSA Conditional Constant Propagation.  If NONZERO_P,
+   record nonzero bits.  */
 
 static unsigned int
-do_ssa_ccp (void)
+do_ssa_ccp (bool nonzero_p)
 {
   unsigned int todo = 0;
   calculate_dominance_info (CDI_DOMINATORS);
   ccp_initialize ();
   ssa_propagate (ccp_visit_stmt, ccp_visit_phi_node);
-  if (ccp_finalize ())
+  if (ccp_finalize (nonzero_p))
     todo = (TODO_cleanup_cfg | TODO_update_ssa);
   free_dominance_info (CDI_DOMINATORS);
   return todo;
@@ -2429,14 +2430,22 @@ class pass_ccp : public gimple_opt_pass
 {
 public:
   pass_ccp (gcc::context *ctxt)
-    : gimple_opt_pass (pass_data_ccp, ctxt)
+    : gimple_opt_pass (pass_data_ccp, ctxt), nonzero_p (false)
   {}
 
   /* opt_pass methods: */
   opt_pass * clone () { return new pass_ccp (m_ctxt); }
+  void set_pass_param (unsigned int n, bool param)
+    {
+      gcc_assert (n == 0);
+      nonzero_p = param;
+    }
   virtual bool gate (function *) { return flag_tree_ccp != 0; }
-  virtual unsigned int execute (function *) { return do_ssa_ccp (); }
+  virtual unsigned int execute (function *) { return do_ssa_ccp (nonzero_p); }
 
+ private:
+  /* Determines whether the pass instance records nonzero bits.  */
+  bool nonzero_p;
 }; // class pass_ccp
 
 } // anon namespace

^ permalink raw reply	[flat|nested] 23+ messages in thread

end of thread, other threads:[~2015-11-16 13:05 UTC | newest]

Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-11-12 11:38 [RFC] Remove first_pass_instance from pass_vrp Tom de Vries
2015-11-12 12:26 ` Richard Biener
2015-11-12 13:49   ` Tom de Vries
2015-11-12 14:05     ` Richard Biener
2015-11-12 14:06       ` Richard Biener
2015-11-12 15:34         ` David Malcolm
2015-11-13 10:35           ` Richard Biener
2015-11-13 13:58             ` [PATCH] " Tom de Vries
2015-11-13 14:12               ` David Malcolm
2015-11-13 14:19                 ` David Malcolm
2015-11-13 14:41                 ` Tom de Vries
2015-11-14  9:19               ` Tom de Vries
2015-11-15 10:56                 ` [PATCH, 0/6] Remove first_pass_instance Tom de Vries
2015-11-15 11:00                   ` [PATCH, 2/6] Remove first_pass_instance from pass_reassoc Tom de Vries
2015-11-15 11:02                   ` [PATCH, 3/6] Remove first_pass_instance from pass_dominator Tom de Vries
2015-11-15 11:03                   ` [PATCH, 4/6] Remove first_pass_instance from pass_object_sizes Tom de Vries
2015-11-15 11:06                   ` [PATCH, 5/6] Remove first_pass_instance from pass_ccp Tom de Vries
2015-11-15 11:09                   ` [PATCH, 6/6] Remove first_pass_instance Tom de Vries
2015-11-15 15:24                     ` David Malcolm
2015-11-15 16:23                       ` Tom de Vries
2015-11-15 22:55                         ` Tom de Vries
2015-11-16 12:09                   ` [PATCH, 0/6] " Bernd Schmidt
2015-11-16 13:05                     ` Tom de Vries

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).