[PATCH] Remove first_pass_instance from pass_vrp
Tom de Vries
Tom_deVries@mentor.com
Fri Nov 13 13:58:00 GMT 2015
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-Remove-first_pass_instance-from-pass_vrp.patch
Type: text/x-patch
Size: 7920 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20151113/658a64d9/attachment.bin>
More information about the Gcc-patches
mailing list