| Message ID | c4fc78d3-4f48-75af-ca32-0664a1309733@suse.de |
|---|---|
| State | New |
| Headers | show |
| Series | Fix PR116650: check all regs in regrename targets | expand |
> Am 10.10.2024 um 16:56 schrieb Michael Matz <matz@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
Hi, On Thu, 10 Oct 2024, Richard Biener wrote: > > 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)) > > Can you please open a bugreport tracking this? PR116850. > The patch is OK. 85bee4f7, thanks. Ciao, Michael.
Hello again, On Thu, 10 Oct 2024, Michael Matz wrote: > > Can you please open a bugreport tracking this? > > PR116850. Gah, too many tabs :) PR117064 I meant. Ciao, Michael.
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; }
(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. 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(-)