Re: [RFC] Remove first_pass_instance from pass_vrp

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

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]