On 9/7/21 2:59 PM, Richard Biener wrote:
> On September 7, 2021 12:02:27 PM GMT+02:00, Aldy Hernandez <aldyh@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.

Agreed.

OK for trunk?

Aldy