diff mbox

Remove first_pass_instance from pass_vrp

Message ID 5645EC5A.9060005@mentor.com
State New
Headers show

Commit Message

Tom de Vries Nov. 13, 2015, 1:57 p.m. UTC
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

Comments

David Malcolm Nov. 13, 2015, 2:12 p.m. UTC | #1
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
David Malcolm Nov. 13, 2015, 2:19 p.m. UTC | #2
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
Tom de Vries Nov. 13, 2015, 2:40 p.m. UTC | #3
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
Tom de Vries Nov. 14, 2015, 9:19 a.m. UTC | #4
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
Tom de Vries Nov. 15, 2015, 10:55 a.m. UTC | #5
[ 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
Bernd Schmidt Nov. 16, 2015, 12:09 p.m. UTC | #6
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
diff mbox

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