Hello Richard:
On 19/06/24 12:52 pm, Ajit Agarwal wrote:
> Hello Richard:
>
> On 19/06/24 2:01 am, Richard Sandiford wrote:
>> Ajit Agarwal <aagar...@linux.ibm.com> writes:
>>> Hello Richard:
>>>
>>> On 14/06/24 4:26 pm, Richard Sandiford wrote:
>>>> Ajit Agarwal <aagar...@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.
>
Other fixes would be to move the code to call process_uses_of_deleted_def
after apply_changes_to_insn where the instruction we delete the
definition.
process_uses_of_deleted_def should be after that.
// Apply the changes to the underlying insn_infos.
for (insn_change *change : changes)
apply_changes_to_insn (*change, new_sets);
// Remove all definitions that are no longer needed. After the above,
// the only uses of such definitions should be dead phis and now-redundant
// live-out uses.
//
// In particular, this means that consumers must handle debug
// instructions before removing a set.
for (insn_change *change : changes)
for (def_info *def : change->old_defs ())
if (def->m_has_been_superceded)
{
auto *set = dyn_cast<set_info *> (def);
if (set && set->has_any_uses ())
process_uses_of_deleted_def (set);
remove_def (def);
}
Is this okay. It worked fine.
>> Thanks,
>> Richard
>
> Thanks & Regards
> Ajit
>
Thanks & Regards
Ajit
