Richard Biener <richard.guent...@gmail.com> writes:
> On Wed, Aug 20, 2025 at 1:03 PM Richard Sandiford
> <richard.sandif...@arm.com> wrote:
>>
>> While testing a later patch, I found that create_degenerate_phi
>> had an inverted test for bitmap_set_bit. It was assuming that
>> the return value was the previous bit value, rather than a
>> "something changed" value. :(
>>
>> Also, the call to add_live_out_use shouldn't be conditional
>> on the DF_LR_OUT operation, since the register could be live-out
>> because of uses later in the same EBB (which do not require a
>> live-out use to be added to the rtl-ssa instruction). Instead,
>> add_live_out should itself check whether a live-out use already exists.
>>
>> Tested on aarch64-linux-gnu, powerpc64le-linux-gnu and
>> x86_64-linux-gnu. OK to install?
>
> OK. Is the inverted test worth backporting?
I was wondering that as well. I'm a bit nervous about backporting
just the removal of !, since without the other part of the fix,
it could just be substituting one problem for another. But the
combination of this patch and the find_use patch might be worth
backporting if there is no fallout.
So how about backporting both patches to GCC 14 and GCC 15 if there
is no fallout in the next couple of weeks? I'm reluctant to change
GCC 13 without a motivating testcase.
Thanks,
Richard
>
>> Richard
>>
>>
>> gcc/
>> * rtl-ssa/blocks.cc (function_info::create_degenerate_phi): Fix
>> inverted test of bitmap_set_bit. Call add_live_out_use even
>> if the register was previously live-out from the predecessor block.
>> Instead...
>> (function_info::add_live_out_use): ...check here whether a live-out
>> use already exists.
>> ---
>> gcc/rtl-ssa/blocks.cc | 11 +++++------
>> 1 file changed, 5 insertions(+), 6 deletions(-)
>>
>> diff --git a/gcc/rtl-ssa/blocks.cc b/gcc/rtl-ssa/blocks.cc
>> index 953fd9e516e..a57b9e15f13 100644
>> --- a/gcc/rtl-ssa/blocks.cc
>> +++ b/gcc/rtl-ssa/blocks.cc
>> @@ -315,15 +315,14 @@ function_info::add_live_out_use (bb_info *bb, set_info
>> *def)
>>
>> // If the end of the block already has an artificial use, that use
>> // acts to make DEF live at the appropriate point.
>> - use_info *use = def->last_nondebug_insn_use ();
>> - if (use && use->insn () == bb->end_insn ())
>> + if (find_use (def, bb->end_insn ()).matching_use ())
>> return;
>>
>> // Currently there is no need to maintain a backward link from the end
>> // instruction to the list of live-out uses. Such a list would be
>> // expensive to update if it was represented using the usual insn_info
>> // access arrays.
>> - use = allocate<use_info> (bb->end_insn (), def->resource (), def);
>> + auto *use = allocate<use_info> (bb->end_insn (), def->resource (), def);
>> use->set_is_live_out_use (true);
>> add_use (use);
>> }
>> @@ -540,12 +539,12 @@ function_info::create_degenerate_phi (ebb_info *ebb,
>> set_info *def)
>> basic_block pred_cfg_bb = single_pred (ebb->first_bb ()->cfg_bb ());
>> bb_info *pred_bb = this->bb (pred_cfg_bb);
>>
>> - if (!bitmap_set_bit (DF_LR_IN (ebb->first_bb ()->cfg_bb ()), regno))
>> + if (bitmap_set_bit (DF_LR_IN (ebb->first_bb ()->cfg_bb ()), regno))
>> {
>> // The register was not previously live on entry to EBB and
>> // might not have been live on exit from PRED_BB either.
>> - if (bitmap_set_bit (DF_LR_OUT (pred_cfg_bb), regno))
>> - add_live_out_use (pred_bb, def);
>> + bitmap_set_bit (DF_LR_OUT (pred_cfg_bb), regno);
>> + add_live_out_use (pred_bb, def);
>> }
>> else
>> {
>> --
>> 2.43.0
>>
