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 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.
> For:
>
>> +// Set subreg with use of INSN given SRC rtx instruction.
>> +static void
>> +set_load_subreg (insn_info *i1, rtx src)
>> +{
>> + rtx set = single_set (i1->rtl());
>> + rtx old_dest = SET_DEST (set);
>> +
>> + for (auto def : i1->defs ())
>> + {
>> + auto set = dyn_cast<set_info *> (def);
>> + for (auto use : set->nondebug_insn_uses ())
>> + {
>> + insn_info *info = use->insn ();
>> + if (!info || !info->rtl ())
>> + continue;
>
> I think this should check use->is_artificial () instead. If that's false,
> then use->insn () and use->insn ()->rtl () should both be nonnull, and there
> should be no need to check.
>
Sure I will do that.
>> +
>> + 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.
> Thanks,
> Richard
Thanks & Regards
Ajit
