Re: [PATCH 02/11] Handle epilogues that contain jumps

[Jeff Law <jeffreyalaw@gmail.com>]

On 10/17/23 03:19, Richard Biener wrote:
> On Thu, Oct 12, 2023 at 10:15 AM Richard Sandiford
> <richard.sandiford@arm.com> wrote:
>>
>> Richard Biener <richard.guenther@gmail.com> writes:
>>> On Tue, Aug 22, 2023 at 12:42 PM Szabolcs Nagy via Gcc-patches
>>> <gcc-patches@gcc.gnu.org> wrote:
>>>>
>>>> From: Richard Sandiford <richard.sandiford@arm.com>
>>>>
>>>> The prologue/epilogue pass allows the prologue sequence
>>>> to contain jumps.  The sequence is then partitioned into
>>>> basic blocks using find_many_sub_basic_blocks.
>>>>
>>>> This patch treats epilogues in the same way.  It's needed for
>>>> a follow-on aarch64 patch that adds conditional code to both
>>>> the prologue and the epilogue.
>>>>
>>>> Tested on aarch64-linux-gnu (including with a follow-on patch)
>>>> and x86_64-linux-gnu.  OK to install?
>>>>
>>>> Richard
>>>>
>>>> gcc/
>>>>          * function.cc (thread_prologue_and_epilogue_insns): Handle
>>>>          epilogues that contain jumps.
>>>> ---
>>>>
>>>> This is a previously approved patch that was not committed
>>>> because it was not needed at the time, but i'd like to commit
>>>> it as it is needed for the followup aarch64 eh_return changes:
>>>>
>>>> https://gcc.gnu.org/pipermail/gcc-patches/2022-November/605769.html
>>>>
>>>> ---
>>>>   gcc/function.cc | 10 ++++++++++
>>>>   1 file changed, 10 insertions(+)
>>>>
>>>> diff --git a/gcc/function.cc b/gcc/function.cc
>>>> index dd2c1136e07..70d1cd65303 100644
>>>> --- a/gcc/function.cc
>>>> +++ b/gcc/function.cc
>>>> @@ -6120,6 +6120,11 @@ thread_prologue_and_epilogue_insns (void)
>>>>                    && returnjump_p (BB_END (e->src)))
>>>>                  e->flags &= ~EDGE_FALLTHRU;
>>>>              }
>>>> +
>>>> +         auto_sbitmap blocks (last_basic_block_for_fn (cfun));
>>>> +         bitmap_clear (blocks);
>>>> +           bitmap_set_bit (blocks, BLOCK_FOR_INSN (epilogue_seq)->index);
>>>> +         find_many_sub_basic_blocks (blocks);
>>>>          }
>>>>         else if (next_active_insn (BB_END (exit_fallthru_edge->src)))
>>>>          {
>>>> @@ -6218,6 +6223,11 @@ thread_prologue_and_epilogue_insns (void)
>>>>            set_insn_locations (seq, epilogue_location);
>>>>
>>>>            emit_insn_before (seq, insn);
>>>> +
>>>> +         auto_sbitmap blocks (last_basic_block_for_fn (cfun));
>>>> +         bitmap_clear (blocks);
>>>> +         bitmap_set_bit (blocks, BLOCK_FOR_INSN (insn)->index);
>>>> +         find_many_sub_basic_blocks (blocks);
>>>
>>> I'll note that clearing a full sbitmap to pass down a single basic block
>>> to find_many_sub_basic_blocks is a quite expensive operation.  May I suggest
>>> to add an overload operating on a single basic block?  It's only
>>>
>>>    FOR_EACH_BB_FN (bb, cfun)
>>>      SET_STATE (bb,
>>>                 bitmap_bit_p (blocks, bb->index) ? BLOCK_TO_SPLIT :
>>> BLOCK_ORIGINAL);
>>>
>>> using the bitmap, so factoring the rest of the function and customizing this
>>> walk would do the trick.  Note that the whole function could be refactored to
>>> handle single blocks more efficiently.
>>
>> Sorry for the late reply, but does this look OK?  Tested on
>> aarch64-linux-gnu and x86_64-linux-gnu.
> 
> LGTM, not sure if I'm qualified enough to approve though (I think you
> are more qualified here, so ..)
It looks quite sensible to me.

Jeff