[RFC] Remove first_pass_instance from pass_vrp

[Tom de Vries]

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]