Re: [Patch, rs6000, middle-end] v2: Add implementation for different targets for pair mem fusion

[Ajit Agarwal <aagarwa1@linux.ibm.com>]

Hello Richard:

On 19/06/24 2:01 am, Richard Sandiford wrote:
> Ajit Agarwal <aagarwa1@linux.ibm.com> writes:
>> Hello Richard:
>>
>> On 14/06/24 4:26 pm, Richard Sandiford wrote:
>>> Ajit Agarwal <aagarwa1@linux.ibm.com> writes:
>>>> Hello Richard:
>>>>
>>>> All comments are addressed.
>>>
>>> I don't think this addresses the following comments from the previous
>>> reviews:
>>>
>>> (1) It is not correct to mark existing insn uses as live-out.
>>>     The patch mustn't try to do this.
>>>
>>
>> Addressed in v3 of the patch.
> 
> The new version still tries to circumvent the live-out assert though.
> While the old patch brute-forced the assert to false by setting
> the live-out flag, the new patch just removes the assert.
> 
> Like I said earlier, the assert is showing up a real bug and we
> should fix the bug rather than suppress the assert.
> 
> rtl-ssa live-out uses are somewhat like DF_LIVE_OUT in df.
> They occur at the end of a basic block, in an artificial insn_info
> that does not correspond to an actual rtl insn.
> 
> The comment above process_uses_of_deleted_def says:
> 
> // SET has been deleted.  Clean up all remaining uses.  Such uses are
> // either dead phis or now-redundant live-out uses.
> 
> In other words, if we're removing a definition, all uses in "real"
> debug and non-debug insns must be removed either earlier than the
> definition or at the same time as the definition.  No such uses
> should remain.  The only uses that should be left are phis and
> the fake end-of-block live-out uses that I described above.  These
> uses are just "plumbing" that support something that is now neither
> defined nor used by real instructions.  It's therefore safe to delete
> the plumbing.
> 

In rtl-ssa/changes.cc I calculate live-out for uses based
on DF_LIVE_OUT in new function that I defined as is_live_out().
This function calculates live out for uses based on DF_LIVE_OUT
for uses. If found to be live out and use is not marked as
live_out_use then I mark it as use->set_is_live_out_use (true).
In that case assert is not required.

If not found to be live out and marked as live_out_use false
than we dont need to check with the assert and should go
ahead and remove the use.

Did you find any issues with above implementation in the 
version 3 of the patch.

> Please see the previous discussion about this:
> 
> ------------------------------------------------------------------------
>>>>>> +// Check whether load can be fusable or not.
>>>>>> +// Return true if fuseable otherwise false.
>>>>>> +bool
>>>>>> +rs6000_pair_fusion::fuseable_load_p (insn_info *info)
>>>>>> +{
>>>>>> +  for (auto def : info->defs())
>>>>>> +    {
>>>>>> +      auto set = dyn_cast<set_info *> (def);
>>>>>> +      for (auto use1 : set->nondebug_insn_uses ())
>>>>>> + use1->set_is_live_out_use (true);
>>>>>> +    }
>>>>>
>>>>> What was the reason for adding this loop?
>>>>>
>>>>
>>>> The purpose of adding is to avoid assert failure in gcc/rtl-ssa/changes.cc:252
>>>
>>> That assert is making sure that we don't delete a definition of a
>>> register (or memory) while a real insn still uses it.  If the assert
>>> is firing then something has gone wrong.
>>>
>>> Live-out uses are a particular kind of use that occur at the end of
>>> basic blocks.  It's incorrect to mark normal insn uses as live-out.
>>>
>>> When an assert fails, it's important to understand why the failure
>>> occurs, rather than brute-force the assert condition to true.
>>>
>>
>> The above assert failure occurs when there is a debug insn and its
>> use is not live-out.
> 
> Uses in debug insns are never live-out uses.
> 
> It sounds like the bug is that we're failing to update all debug uses of
> the original register.  We need to do that, or "reset" the debug insn if
> substitution fails for some reason.
> 
> See fixup_debug_uses for what the target-independent part of the pass
> does for debug insns that are affected by movement.  Hopefully the
> update needed here will be simpler than that.
> ------------------------------------------------------------------------
> 
> What happens if you leave the assert alone?  When does it fire?  Is it
> still for uses in debug insns?  If so, it's the fusion pass's responsibility
> to update those, as mentioned above.  And it must update them before,
> or at the same time as, it deletes the definition.
>

For debug insn I call reset_debug_use and now I dont see issues
with debug insn and issues I see with  non debug insn where
def is there in old_defs and use has to be removed for the insn
that we modify load with OO UNSPEC to generate lxvp.
 
In rtl-ssa/blocks.cc we calculate live out based on DF_LIVE_OUT
and mark use->set_is_live_out (true).

But for cases that are not live out we dont mark them live out
to be true in blocks.cc

Similarly I am doing in version v3 of the patch (in rtl-ssa/changes.cc) with is_live_out() new function wherein I calculate live out based on DF_LIVE_OUT and for live out found, assert is not required as it always true and if not live out we should just go ahead and remove use and assert is not required.

> Thanks,
> Richard

Thanks & Regards
Ajit