Re: [RFC] Remove first_pass_instance from pass_vrp

[Richard Biener <richard dot guenther at gmail dot com>]

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
>