[RFC] Remove first_pass_instance from pass_vrp
diff mbox

Message ID CAFiYyc1vpb_nU_ip9sk69P0P9r2rnBsOOWgJ-j-T9eMk5m3Xqw@mail.gmail.com
State New
Headers show

Commit Message

Richard Biener Nov. 12, 2015, 2:06 p.m. UTC
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,

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]

Comments

David Malcolm Nov. 12, 2015, 3:33 p.m. UTC | #1
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
Richard Biener Nov. 13, 2015, 10:35 a.m. UTC | #2
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
>

Patch
diff mbox

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