> Am 10.10.2024 um 16:56 schrieb Michael Matz <m...@suse.de>:
>
> (this came up for m68k vs. LRA, but is a generic problem)
>
> Regrename wants to use new registers for certain def-use chains.
> For validity of replacements it needs to check that the selected
> candidates are unused up to then. That's done in check_new_reg_p.
> But if it so happens that the new register needs more hardregs
> than the old register (which happens if the target allows inter-bank
> moves and the mode is something like a DFmode that needs to be placed
> into a SImode reg-pair), then check_new_reg_p only checks the
> first of those registers for free-ness.
>
> This is caused by that function looking up the number of necessary
> hardregs only in terms of the old hardreg number. It of course needs
> to do that in terms of the new candidate regnumber. The symptom is that
> regrename sometimes clobbers the higher numbered registers of such a
> regrename target pair. This patch fixes that problem.
>
> (In the particular case of the bug report it was LRA that left over a
> inter-bank move instruction that triggers regrename, ultimately causing
> the mis-compile. Reload didn't do that, but in general we of course
> can't rely on such moves not happening if the target allows them.)
>
> This also shows a general confusion in that function and the target hook
> interface here:
>
> for (i = nregs - 1; i >= 0; --)
> ...
> || ! HARD_REGNO_RENAME_OK (reg + i, new_reg + i))
>
> it uses nregs in a way that requires it to be the same between old and
> new register. The problem is that the target hook only gets register
> numbers, when it instead should get a mode and register numbers and
> would be called only for the first but not for subsequent registers.
> I've looked at a number of definitions of that target hook and I think
> that this is currently harmless in the sense that it would merely rule
> out some potential reg-renames that would in fact be okay to do. So I'm
> not changing the target hook interface here and hence that problem
> remains unfixed.
Can you please open a bugreport tracking this?
The patch is OK.
Thanks,
Richard
> PR rtl-optimization/116650
> * regrename.cc (check_new_reg_p): Calculate nregs in terms of
> the new candidate register.
> ---
>
> Regstrapped on x86-64-linux, okay for trunk?
>
>
> Ciao,
> Michael.
>
> ---
> gcc/regrename.cc | 25 +++++++++++++++++++------
> 1 file changed, 19 insertions(+), 6 deletions(-)
>
> diff --git a/gcc/regrename.cc b/gcc/regrename.cc
> index 054e601740b..22668d7bf57 100644
> --- a/gcc/regrename.cc
> +++ b/gcc/regrename.cc
> @@ -324,10 +324,27 @@ static bool
> check_new_reg_p (int reg ATTRIBUTE_UNUSED, int new_reg,
> class du_head *this_head, HARD_REG_SET this_unavailable)
> {
> - int nregs = this_head->nregs;
> + int nregs = 1;
> int i;
> struct du_chain *tmp;
>
> + /* See whether new_reg accepts all modes that occur in
> + definition and uses and record the number of regs it would take. */
> + for (tmp = this_head->first; tmp; tmp = tmp->next_use)
> + {
> + int n;
> + /* Completely ignore DEBUG_INSNs, otherwise we can get
> + -fcompare-debug failures. */
> + if (DEBUG_INSN_P (tmp->insn))
> + continue;
> +
> + if (!targetm.hard_regno_mode_ok (new_reg, GET_MODE (*tmp->loc)))
> + return false;
> + n = hard_regno_nregs (new_reg, GET_MODE (*tmp->loc));
> + if (n > nregs)
> + nregs = n;
> + }
> +
> for (i = nregs - 1; i >= 0; --i)
> if (TEST_HARD_REG_BIT (this_unavailable, new_reg + i)
> || fixed_regs[new_reg + i]
> @@ -348,14 +365,10 @@ check_new_reg_p (int reg ATTRIBUTE_UNUSED, int new_reg,
> definition and uses. */
> for (tmp = this_head->first; tmp; tmp = tmp->next_use)
> {
> - /* Completely ignore DEBUG_INSNs, otherwise we can get
> - -fcompare-debug failures. */
> if (DEBUG_INSN_P (tmp->insn))
> continue;
>
> - if (!targetm.hard_regno_mode_ok (new_reg, GET_MODE (*tmp->loc))
> - || call_clobbered_in_chain_p (this_head, GET_MODE (*tmp->loc),
> - new_reg))
> + if (call_clobbered_in_chain_p (this_head, GET_MODE (*tmp->loc),
> new_reg))
> return false;
> }
>
> --
> 2.39.1
