Hello Richard:
On 18/07/24 4:44 pm, Richard Sandiford wrote:
> Ajit Agarwal <aagar...@linux.ibm.com> writes:
>> [...]
>>>> +// Set subreg for OO mode pair to generate sequential registers given
>>>> +// insn_info pairs I1, I2 and LOAD_P is true iff load insn and false
>>>> +// if store insn.
>>>> +void
>>>> +rs6000_pair_fusion::set_multiword_subreg (insn_info *i1, insn_info *i2,
>>>> + bool load_p)
>>>> +{
>>>> + if (load_p)
>>>> + {
>>>> + bool i1_subreg_p = use_has_subreg_p (i1);
>>>> + bool i2_subreg_p = use_has_subreg_p (i2);
>>>> +
>>>> + if (i1_subreg_p || i2_subreg_p)
>>>> + set_multiword_existing_subreg (i1, i2);
>>>> + else
>>>> + set_multiword_subreg_load (i1, i2);
>>>
>>> I don't understand this. Why do we have both set_multiword_existing_subreg
>>> and set_multiword_subreg_load? i1_subreg_p and i2_subreg_p are logically
>>> independent of one another (since i1 and i2 were separate instructions
>>> until now). So "i1_subreg_p || i2_subreg_p" implies that
>>> set_multiword_existing_subreg can handle i1s that have no existing
>>> subreg (used when i2_subreg_p) and that it can handle i2s that have no
>>> existing subreg (used when i1_subreg_p). So doesn't this mean that
>>> set_multiword_existing_subreg can handle everything?
>>>
>>
>> I will make the following change.
>> if (load_p)
>> {
>> bool i1_subreg_p = use_has_subreg_p (i1);
>> bool i2_subreg_p = use_has_subreg_p (i2);
>>
>> if (!i1_subreg_p && !i2_subreg_p)
>> set_multiword_subreg_load (i1, i2);
>> else
>> set_multiword_existing_subreg (i1, i2);
>> }
>>
>> Is this okay.
>
> That's the same thing though: it's just replacing a ? A : B with !a ? B : A.
>
Addressed in v7 of the patch.
>>> IMO, the way the update should work is that:
>>>
>>> (a) all references to the old registers should be updated via
>>> insn_propagation (regardless of whether the old references
>>> involved subregs).
>>>
>>> (b) those updates should be part of the same insn_change group as
>>> the change to the load itself.
>>>
>>> For stores, definitions of the stored register can probably be handled
>>> directly using df_refs, but there too, the updates should IMO be part
>>> of the same insn_change group as the change to the store itself.
>>>
>>> In both cases, it's the:
>>>
>>> crtl->ssa->change_insns (changes);
>>>
>>> in pair_fusion_bb_info::fuse_pair that should be responsible for
>>> updating the rtl-ssa IR. The changes that the pass wants to make
>>> should be described as insn_changes and passed to change_insns.
>>>
>>> The reason for funneling all changes through change_insns is that
>>> it allows rtl-ssa to maintain more complex datastructures. Clients
>>> aren't supposed to manually update the datastructures piecemeal.
>>>
>>
>> I am afraid I am not getting this. Would you mind elaborating this.
>> Sorry for that.
>
> See how fwprop.cc makes changes. It:
>
> - creates an insn_change for each change that it wants to make
>
> - uses insn_propagation to replace the old value with the new value
>
> - sets the new_uses of the insn_change to reflect the effect
> of the propagation (in this case, replacing the old 128-bit
> value with a 256-bit value)
>
> - uses change_insn to commit the change
>
> The process would be similar here.
>
Addressed in v7 of the patch.
> Thanks,
> Richard
Thanks & Regards
Ajit
