On September 7, 2021 12:02:27 PM GMT+02:00, Aldy Hernandez <al...@redhat.com>
wrote:
>
>
>On 9/6/21 9:19 AM, Richard Biener wrote:
>> On Fri, Sep 3, 2021 at 3:59 PM Aldy Hernandez via Gcc-patches
>> <gcc-patches@gcc.gnu.org> wrote:
>>>
>>> This patch abstracts out a couple common idioms in the forward
>>> threader that I found useful while navigating the code base.
>>>
>>> Tested on x86-64 Linux.
>>>
>>> OK?
>>>
>>> gcc/ChangeLog:
>>>
>>> * tree-ssa-threadedge.c (has_phis_p): New.
>>> (forwarder_block_p): New.
>>> (potentially_threadable_block): Call forwarder_block_p.
>>> (jump_threader::thread_around_empty_blocks): Call has_phis_p.
>>> (jump_threader::thread_through_normal_block): Call
>>> forwarder_block_p.
>>> ---
>>> gcc/tree-ssa-threadedge.c | 25 +++++++++++++++++++------
>>> 1 file changed, 19 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/gcc/tree-ssa-threadedge.c b/gcc/tree-ssa-threadedge.c
>>> index e57f6d3e39c..3db54a199fd 100644
>>> --- a/gcc/tree-ssa-threadedge.c
>>> +++ b/gcc/tree-ssa-threadedge.c
>>> @@ -95,6 +95,21 @@ jump_threader::thread_through_all_blocks (bool
>>> may_peel_loop_headers)
>>> return m_registry->thread_through_all_blocks (may_peel_loop_headers);
>>> }
>>>
>>> +static inline bool
>>> +has_phis_p (basic_block bb)
>>> +{
>>> + return !gsi_end_p (gsi_start_phis (bb));
>>
>> gimple_seq_empty_p (phi_nodes (bb)) shoud be cheaper. Do virtual PHIs
>> count as PHIs for you?
>
>I don't know. The goal was to abstract some common idioms without
>changing existing behavior, but if my abstractions confuse other
>readers, perhaps I should revert my patch.
>
>FWIW, my initial motivation here was to merge the path profitability
>code between the forward and backward threaders. It seems the forward
>threader is more permissive than the backward threader, even though the
>latter can thread more paths than it's allowed (per profitable_path_p).
>
>>
>>> +}
>>> +
>>> +/* Return TRUE for a forwarder block which is defined as having PHIs
>>> + but no instructions. */
>>> +
>>> +static bool
>>> +forwarder_block_p (basic_block bb)
>>
>> There exists a function with exactly the same signature in cfgrtl.h, likewise
>> several similar implementations might exist elsewhere.
>
>Ughh, that's definitely not good.
>
>>
>> Your definition is also quite odd, not matching what one would expect
>> (the PHI requirement). The tree-cfgcleanup.c variant has
>> tree_forwarder_block_p which is explicit about this.
>>
>> Btw, gsi_start_nondebug_bb does not ignore labels.
>
>Would a name like empty_block_with_phis_p be more appropriate?
I think so. That said, my main concern ist the clash with the same named
function.
Richard.
>Aldy
>
