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