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.
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.
Thanks,
Richard
