Ajit Agarwal <aagar...@linux.ibm.com> writes:
> Hello Richard:
>
> On 15/08/24 3:45 pm, Richard Sandiford wrote:
>> Ajit Agarwal <aagar...@linux.ibm.com> writes:
>>> +static void
>>> +update_change (set_info *set)
>>> +{
>>> + if (!set->has_any_uses ())
>>> + return;
>>> +
>>> + auto *use = *set->all_uses ().begin ();
>>> + do
>>> + {
>>> + auto *next_use = use->next_use ();
>>> + if (use->is_in_debug_insn ())
>>> + substitute_debug_use (use);
>>> + else if (use->is_in_phi ())
>>> + {
>>> + update_change (use->phi ());
>>> + }
>>> + else
>>> + {
>>> + crtl->ssa->remove_use (use);
>>> + }
>>> + use = next_use;
>>> + }
>>> + while (use);
>>> +}
>>
>> This still contains direct modifications to the rtl-ssa ir (remove_use).
>> Which case is it handling? REG_EQUAL notes?
>>
>
> This is done to handle your below comments:
>
> Thanks. It looks like you're updating just the definitions,
> and then later updating the uses. That isn't the way that rtl-ssa
> is supposed to be used. Each change set -- in other words, each call
> to function_info::change_insns -- must go from a valid state to a valid
> state. That is, the RTL must be self-consistent before every individual
> call to function_info::change_insns and must be self-consistent after
> every individual call to function_info::change_insns.
>
> This is what I meant before about:
>
> ... 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.
>
> Since you want to update all uses of register 178, you need to include
> those updates in the same change set as the change to insns 130 and 131,
> rather than doing them later.
The patch isn't using function_info::change_insns though. It's calling
the private remove_use function directly.
>> The patch shouldn't rely on:
>>
>>> diff --git a/gcc/rtl-ssa/functions.h b/gcc/rtl-ssa/functions.h
>>> index 567701c7995..3003251e62c 100644
>>> --- a/gcc/rtl-ssa/functions.h
>>> +++ b/gcc/rtl-ssa/functions.h
>>> @@ -222,6 +222,13 @@ public:
>>> template<typename T, typename... Ts>
>>> T *change_alloc (obstack_watermark &wm, Ts... args);
>>>
>>> + auto_vec<access_info *> &get_m_temp_defs () { return m_temp_defs; }
>>> +
>>> + template<typename T, typename... Ts>
>>> + T *allocate (Ts... args);
>>> +
>>> + void remove_use (use_info *);
>>> +
>>> private:
>>> class bb_phi_info;
>>> class build_info;
>>
>> Those aren't things that rs6000 code should be doing directly.
>>
> Would you mind explaining where to handle this.
Each change to an instruction should be done via function_info::change_insns.
The idea is that change should be structured as follows:
(A) Call crtl->ssa->new_change_attempt ()
(B) For each instruction that you want to change:
(1) Create an insn_change.
(2) In that insn_change, describe the new uses and defs, and the
range of possible instruction positions (if the instruction is
allowed to move).
(3) Use validate_change/insn_propagation/etc. to change the RTL of
the instruction.
[(2) and (3) could be done in the opposite order]
At this stage, nothing is committed. No insn_infos have changed and
all insn patterns can be reset to their original state by cancel_changes
(which happens automatically when an uncommitted change attempt goes out
of scope).
Next:
(C) Check whether the set of changes are valid and self-consistent.
Also check whether they are a win. If no to either, abort the change.
(D) Call function_info::change_insns to commit and finalise the changes.
All the logic for updating "permanent" rtl-ssa structures should go in
function_info::change_insns. It shouldn't be done by pair-fusion itself,
or by target code.
pair-fusion already works like this. I think the rs6000 pass just needs
to hook into (B), so that it can update the uses fed by loads and the
definitions that feed stores.
>>> +
>>> + rtx_insn *rtl_insn = info->rtl ();
>>> + insn_propagation prop (rtl_insn, old_dest, src);
>>> + if (!prop.apply_to_pattern (&PATTERN (rtl_insn)))
>>> + gcc_assert (0);
>>> + }
>>> + }
>>> +}
>>
>> Could you combine this with the code that creates the insn_change
>> for the insn, rather than doing that in a separate function?
>> IMO it's better to keep the creation of insn_changes together
>> with the changes that they describe.
>>
>
> Sorry I didn't get this. Would you mind explaining where to make
> the above change.
The above answer is supposed to cover this too. Each change to a use
insn should involve:
- creating an insn_change
- using insn_propagation to change the rtl
- working out the new uses for the insn, and storing them in the
insn_change (not the insn itself)
- adding the insn_change to the list of changes that pair-fusion performs
for the load fusion.
Then, the loads and their uses will be updated in one go, by one call
to function_info::change_insns.
Please let me know if the explanation doesn't make sense. It's difficult
to spot the unstated assumptions when explaining one's own code. :)
Thanks,
Richard
