Re: [PATCH] Abstract PHI and forwarder block checks in jump threader.

[Jeff Law <jeffreyalaw@gmail.com>]

On 9/7/2021 7:23 AM, Aldy Hernandez wrote:
>
>
> 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
>
>
> p.patch
>
> commit 77ac56456d5db150d6a71eaca918f19d2b478f82
> Author: Aldy Hernandez <aldyh@redhat.com>
> Date:   Tue Sep 7 15:20:23 2021 +0200
>
>      Rename forwarder_block_p in treading code to empty_block_with_phis_p.
>      
>      gcc/ChangeLog:
>      
>              * tree-ssa-threadedge.c (forwarder_block_p): Rename to...
>              (empty_block_with_phis_p): ...this.
>              (potentially_threadable_block): Same.
>              (jump_threader::thread_through_normal_block): Same.
OK.  I nearly called out a request for a name change....  Guess I should 
have :-)

jeff