* [PATCH] Fix IRA register preferencing
@ 2015-11-10 13:31 Wilco Dijkstra
2015-11-12 3:44 ` Vladimir Makarov
0 siblings, 1 reply; 7+ messages in thread
From: Wilco Dijkstra @ 2015-11-10 13:31 UTC (permalink / raw)
To: gcc-patches
Ping of https://gcc.gnu.org/ml/gcc-patches/2014-12/msg00829.html:
This fixes a bug in register preferencing. When live range splitting creates
a new register from
another, it copies most fields except for the register preferences. The
preference GENERAL_REGS is
used as reg_pref[i].prefclass is initialized with GENERAL_REGS in
allocate_reg_info () and
resize_reg_info ().
This initialization value is not innocuous like the comment suggests - if a
new register has a
non-integer mode, it is forced to prefer GENERAL_REGS. This changes the
register costs in pass 2 so
that they are incorrect. As a result the liverange is either spilled or
allocated to an integer
register:
void g(double);
void f(double x)
{
if (x == 0)
return;
g (x);
g (x);
}
f:
fcmp d0, #0.0
bne .L6
ret
.L6:
stp x19, x30, [sp, -16]!
fmov x19, d0
bl g
fmov d0, x19
ldp x19, x30, [sp], 16
b g
With the fix it uses a floating point register as expected. Given a similar
issue in
https://gcc.gnu.org/ml/gcc-patches/2014-11/msg02253.html, would it not be
better to change the
initialization values of reg_pref to illegal register classes so this kind
of issue can be trivially
found with an assert? Also would it not be a good idea to have a single
register copy function that
ensures all data is copied?
ChangeLog: 2014-12-09 Wilco Dijkstra wdijkstr@arm.com
* gcc/ira-emit.c (ira_create_new_reg): Copy preference classes.
---
gcc/ira-emit.c | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/gcc/ira-emit.c b/gcc/ira-emit.c
index d246b7f..d736836 100644
--- a/gcc/ira-emit.c
+++ b/gcc/ira-emit.c
@@ -348,6 +348,7 @@ rtx
ira_create_new_reg (rtx original_reg)
{
rtx new_reg;
+ int original_regno = REGNO (original_reg);
new_reg = gen_reg_rtx (GET_MODE (original_reg));
ORIGINAL_REGNO (new_reg) = ORIGINAL_REGNO (original_reg);
@@ -356,8 +357,16 @@ ira_create_new_reg (rtx original_reg)
REG_ATTRS (new_reg) = REG_ATTRS (original_reg);
if (internal_flag_ira_verbose > 3 && ira_dump_file != NULL)
fprintf (ira_dump_file, " Creating newreg=%i from oldreg=%i\n",
- REGNO (new_reg), REGNO (original_reg));
+ REGNO (new_reg), original_regno);
ira_expand_reg_equiv ();
+
+ /* Copy the preference classes to new_reg. */
+ resize_reg_info ();
+ setup_reg_classes (REGNO (new_reg),
+ reg_preferred_class (original_regno),
+ reg_alternate_class (original_regno),
+ reg_allocno_class (original_regno));
+
return new_reg;
}
--
1.9.1
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Fix IRA register preferencing
2015-11-10 13:31 [PATCH] Fix IRA register preferencing Wilco Dijkstra
@ 2015-11-12 3:44 ` Vladimir Makarov
0 siblings, 0 replies; 7+ messages in thread
From: Vladimir Makarov @ 2015-11-12 3:44 UTC (permalink / raw)
To: Wilco Dijkstra, gcc-patches
On 11/10/2015 08:30 AM, Wilco Dijkstra wrote:
> Ping of https://gcc.gnu.org/ml/gcc-patches/2014-12/msg00829.html:
>
>
>
> This fixes a bug in register preferencing. When live range splitting creates
> a new register from
> another, it copies most fields except for the register preferences. The
> preference GENERAL_REGS is
> used as reg_pref[i].prefclass is initialized with GENERAL_REGS in
> allocate_reg_info () and
> resize_reg_info ().
>
> This initialization value is not innocuous like the comment suggests - if a
> new register has a
> non-integer mode, it is forced to prefer GENERAL_REGS. This changes the
> register costs in pass 2 so
> that they are incorrect. As a result the liverange is either spilled or
> allocated to an integer
> register:
>
> void g(double);
> void f(double x)
> {
> if (x == 0)
> return;
> g (x);
> g (x);
> }
>
> f:
> fcmp d0, #0.0
> bne .L6
> ret
> .L6:
> stp x19, x30, [sp, -16]!
> fmov x19, d0
> bl g
> fmov d0, x19
> ldp x19, x30, [sp], 16
> b g
>
> With the fix it uses a floating point register as expected. Given a similar
> issue in
> https://gcc.gnu.org/ml/gcc-patches/2014-11/msg02253.html, would it not be
> better to change the
> initialization values of reg_pref to illegal register classes so this kind
> of issue can be trivially
> found with an assert? Also would it not be a good idea to have a single
> register copy function that
> ensures all data is copied?
Having a function and the assert would be wonderful. If you have a
patch for this, I'll be glad to review it.
If you don't have a patch or have no time or willing to work on it, you
can commit given here patch into the trunk.
Thanks.
>
> ChangeLog: 2014-12-09 Wilco Dijkstra wdijkstr@arm.com
>
> * gcc/ira-emit.c (ira_create_new_reg): Copy preference classes.
>
> ---
> gcc/ira-emit.c | 11 ++++++++++-
> 1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/gcc/ira-emit.c b/gcc/ira-emit.c
> index d246b7f..d736836 100644
> --- a/gcc/ira-emit.c
> +++ b/gcc/ira-emit.c
> @@ -348,6 +348,7 @@ rtx
> ira_create_new_reg (rtx original_reg)
> {
> rtx new_reg;
> + int original_regno = REGNO (original_reg);
>
> new_reg = gen_reg_rtx (GET_MODE (original_reg));
> ORIGINAL_REGNO (new_reg) = ORIGINAL_REGNO (original_reg);
> @@ -356,8 +357,16 @@ ira_create_new_reg (rtx original_reg)
> REG_ATTRS (new_reg) = REG_ATTRS (original_reg);
> if (internal_flag_ira_verbose > 3 && ira_dump_file != NULL)
> fprintf (ira_dump_file, " Creating newreg=%i from oldreg=%i\n",
> - REGNO (new_reg), REGNO (original_reg));
> + REGNO (new_reg), original_regno);
> ira_expand_reg_equiv ();
> +
> + /* Copy the preference classes to new_reg. */
> + resize_reg_info ();
> + setup_reg_classes (REGNO (new_reg),
> + reg_preferred_class (original_regno),
> + reg_alternate_class (original_regno),
> + reg_allocno_class (original_regno));
> +
> return new_reg;
> }
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH] Fix IRA register preferencing
2014-12-10 20:47 ` Jeff Law
@ 2015-04-27 15:01 ` Wilco Dijkstra
0 siblings, 0 replies; 7+ messages in thread
From: Wilco Dijkstra @ 2015-04-27 15:01 UTC (permalink / raw)
To: 'Jeff Law', gcc-patches
> Jeff Law wrote:
> On 12/10/14 06:26, Wilco Dijkstra wrote:
> >
> > If recomputing is best does that mean that record_reg_classes should not
> > give a boost to the preferred class in the 2nd pass?
> Perhaps. I haven't looked deeply at this part of IRA. I was relaying
> my experiences with (ab)using the ira-reload callbacks to handle
> allocation after splitting -- where getting the costs and classes
> updated in a reasonable manner was clearly important to getting good
> code. One could probably argue I should have kept testcases from that
> work :-)
>
>
> I don't understand
> > what purpose this has - if the preferred class is from the first pass, it
> > is already correct, so all it does is boost the preferred class further.
> > And if the preferred class is wrong (eg. after live range splitting), it
> > will boost the incorrect class even harder, so in the end you never get
> > a different class.
> It may be historical from the old regclass code, not really sure.
>
> > From what you're saying, recomputing seems best, and I'd be happy to submit
> > a patch to remove all the preferred class code from record_reg_classes.
> Recomputing certainly helped the cases I was looking at.
> >
> > However there is still the possibility the preferred class is queried before
> > the recomputation happens (I think that is a case Renlin fixed). Either these
> > should be faulted and fixed by forcing recomputation, or we need to provide a
> > correct preferred class. That should be a copy of the original class.
> I believe I had copied the original classes, then recomputed them to
> avoid any uninitialized memory reads and the like. But looking at the
> old branch, I don't see the recomputation for classes (though I do see
> it for costs). Of course all the backwards walk stuff isn't there
> either, so there's clearly code I worked on extensively, but never
> committed...
Well, looking into this further it does look like the preferences are improved
during the 2nd pass (in particular fewer cases of the bad ALL_REGS preference
that causes all the inefficient code), so it looks like recomputing preferences
is not beneficial and my original patch is the right fix for now.
https://gcc.gnu.org/ml/gcc-patches/2014-12/msg00829.html
Wilco
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Fix IRA register preferencing
2014-12-10 13:26 ` Wilco Dijkstra
@ 2014-12-10 20:47 ` Jeff Law
2015-04-27 15:01 ` Wilco Dijkstra
0 siblings, 1 reply; 7+ messages in thread
From: Jeff Law @ 2014-12-10 20:47 UTC (permalink / raw)
To: Wilco Dijkstra, gcc-patches
On 12/10/14 06:26, Wilco Dijkstra wrote:
>
> If recomputing is best does that mean that record_reg_classes should not
> give a boost to the preferred class in the 2nd pass?
Perhaps. I haven't looked deeply at this part of IRA. I was relaying
my experiences with (ab)using the ira-reload callbacks to handle
allocation after splitting -- where getting the costs and classes
updated in a reasonable manner was clearly important to getting good
code. One could probably argue I should have kept testcases from that
work :-)
I don't understand
> what purpose this has - if the preferred class is from the first pass, it
> is already correct, so all it does is boost the preferred class further.
> And if the preferred class is wrong (eg. after live range splitting), it
> will boost the incorrect class even harder, so in the end you never get
> a different class.
It may be historical from the old regclass code, not really sure.
> From what you're saying, recomputing seems best, and I'd be happy to submit
> a patch to remove all the preferred class code from record_reg_classes.
Recomputing certainly helped the cases I was looking at.
>
> However there is still the possibility the preferred class is queried before
> the recomputation happens (I think that is a case Renlin fixed). Either these
> should be faulted and fixed by forcing recomputation, or we need to provide a
> correct preferred class. That should be a copy of the original class.
I believe I had copied the original classes, then recomputed them to
avoid any uninitialized memory reads and the like. But looking at the
old branch, I don't see the recomputation for classes (though I do see
it for costs). Of course all the backwards walk stuff isn't there
either, so there's clearly code I worked on extensively, but never
committed...
Jeff
^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH] Fix IRA register preferencing
2014-12-09 23:17 ` Jeff Law
@ 2014-12-10 13:26 ` Wilco Dijkstra
2014-12-10 20:47 ` Jeff Law
0 siblings, 1 reply; 7+ messages in thread
From: Wilco Dijkstra @ 2014-12-10 13:26 UTC (permalink / raw)
To: 'Jeff Law', gcc-patches
> Jeff Law wrote:
> On 12/09/14 12:21, Wilco Dijkstra wrote:
> > With the fix it uses a floating point register as expected. Given a similar issue in
> > https://gcc.gnu.org/ml/gcc-patches/2014-11/msg02253.html, would it not be better to change
> the
> > initialization values of reg_pref to illegal register classes so this kind of issue can be
> trivially
> > found with an assert? Also would it not be a good idea to have a single register copy
> function that
> > ensures all data is copied?
> But there are other times when you really don't want to copy, say when
> the original had a small class, but the copy can go into a larger class.
>
> I banged my head on this when I was doing similar work on range
> splitting a few years back and ended up recomputing the preferred and
> alternate class information. That was much better than copying the
> classes.
If recomputing is best does that mean that record_reg_classes should not
give a boost to the preferred class in the 2nd pass? I don't understand
what purpose this has - if the preferred class is from the first pass, it
is already correct, so all it does is boost the preferred class further.
And if the preferred class is wrong (eg. after live range splitting), it
will boost the incorrect class even harder, so in the end you never get
a different class.
> I pondered heuristics to expand the preferred class, but never
> implemented anything IIRC. A trivial heuristic would be to bump to the
> next larger class if the given class was a singleton, otherwise copy the
> class.
>
> The obvious counter to that heuristic is an allocno that has two ranges
> (or N ranges) where we would prefer a different singleton class for each
> range. In fact, I'm pretty sure I ran into this kind of situation and
> that led me down the "just recompute it" path.
>
> I'd hazard a guess that the simple heuristic would do better than what
> we're doing now with GENERAL_REGS though or what you're doing with copying.
From what you're saying, recomputing seems best, and I'd be happy to submit
a patch to remove all the preferred class code from record_reg_classes.
However there is still the possibility the preferred class is queried before
the recomputation happens (I think that is a case Renlin fixed). Either these
should be faulted and fixed by forcing recomputation, or we need to provide a
correct preferred class. That should be a copy of the original class.
Wilco
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Fix IRA register preferencing
2014-12-09 19:21 Wilco Dijkstra
@ 2014-12-09 23:17 ` Jeff Law
2014-12-10 13:26 ` Wilco Dijkstra
0 siblings, 1 reply; 7+ messages in thread
From: Jeff Law @ 2014-12-09 23:17 UTC (permalink / raw)
To: Wilco Dijkstra, gcc-patches
On 12/09/14 12:21, Wilco Dijkstra wrote:
> This fixes a bug in register preferencing. When live range splitting creates a new register from
> another, it copies most fields except for the register preferences. The preference GENERAL_REGS is
> used as reg_pref[i].prefclass is initialized with GENERAL_REGS in allocate_reg_info () and
> resize_reg_info ().
>
> This initialization value is not innocuous like the comment suggests - if a new register has a
> non-integer mode, it is forced to prefer GENERAL_REGS. This changes the register costs in pass 2 so
> that they are incorrect. As a result the liverange is either spilled or allocated to an integer
> register:
>
> void g(double);
> void f(double x)
> {
> if (x == 0)
> return;
> g (x);
> g (x);
> }
>
> f:
> fcmp d0, #0.0
> bne .L6
> ret
> .L6:
> stp x19, x30, [sp, -16]!
> fmov x19, d0
> bl g
> fmov d0, x19
> ldp x19, x30, [sp], 16
> b g
>
> With the fix it uses a floating point register as expected. Given a similar issue in
> https://gcc.gnu.org/ml/gcc-patches/2014-11/msg02253.html, would it not be better to change the
> initialization values of reg_pref to illegal register classes so this kind of issue can be trivially
> found with an assert? Also would it not be a good idea to have a single register copy function that
> ensures all data is copied?
But there are other times when you really don't want to copy, say when
the original had a small class, but the copy can go into a larger class.
I banged my head on this when I was doing similar work on range
splitting a few years back and ended up recomputing the preferred and
alternate class information. That was much better than copying the
classes.
I pondered heuristics to expand the preferred class, but never
implemented anything IIRC. A trivial heuristic would be to bump to the
next larger class if the given class was a singleton, otherwise copy the
class.
The obvious counter to that heuristic is an allocno that has two ranges
(or N ranges) where we would prefer a different singleton class for each
range. In fact, I'm pretty sure I ran into this kind of situation and
that led me down the "just recompute it" path.
I'd hazard a guess that the simple heuristic would do better than what
we're doing now with GENERAL_REGS though or what you're doing with copying.
jeff
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] Fix IRA register preferencing
@ 2014-12-09 19:21 Wilco Dijkstra
2014-12-09 23:17 ` Jeff Law
0 siblings, 1 reply; 7+ messages in thread
From: Wilco Dijkstra @ 2014-12-09 19:21 UTC (permalink / raw)
To: gcc-patches
This fixes a bug in register preferencing. When live range splitting creates a new register from
another, it copies most fields except for the register preferences. The preference GENERAL_REGS is
used as reg_pref[i].prefclass is initialized with GENERAL_REGS in allocate_reg_info () and
resize_reg_info ().
This initialization value is not innocuous like the comment suggests - if a new register has a
non-integer mode, it is forced to prefer GENERAL_REGS. This changes the register costs in pass 2 so
that they are incorrect. As a result the liverange is either spilled or allocated to an integer
register:
void g(double);
void f(double x)
{
if (x == 0)
return;
g (x);
g (x);
}
f:
fcmp d0, #0.0
bne .L6
ret
.L6:
stp x19, x30, [sp, -16]!
fmov x19, d0
bl g
fmov d0, x19
ldp x19, x30, [sp], 16
b g
With the fix it uses a floating point register as expected. Given a similar issue in
https://gcc.gnu.org/ml/gcc-patches/2014-11/msg02253.html, would it not be better to change the
initialization values of reg_pref to illegal register classes so this kind of issue can be trivially
found with an assert? Also would it not be a good idea to have a single register copy function that
ensures all data is copied?
ChangeLog: 2014-12-09 Wilco Dijkstra wdijkstr@arm.com
* gcc/ira-emit.c (ira_create_new_reg): Copy preference classes.
---
gcc/ira-emit.c | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/gcc/ira-emit.c b/gcc/ira-emit.c
index d246b7f..d736836 100644
--- a/gcc/ira-emit.c
+++ b/gcc/ira-emit.c
@@ -348,6 +348,7 @@ rtx
ira_create_new_reg (rtx original_reg)
{
rtx new_reg;
+ int original_regno = REGNO (original_reg);
new_reg = gen_reg_rtx (GET_MODE (original_reg));
ORIGINAL_REGNO (new_reg) = ORIGINAL_REGNO (original_reg);
@@ -356,8 +357,16 @@ ira_create_new_reg (rtx original_reg)
REG_ATTRS (new_reg) = REG_ATTRS (original_reg);
if (internal_flag_ira_verbose > 3 && ira_dump_file != NULL)
fprintf (ira_dump_file, " Creating newreg=%i from oldreg=%i\n",
- REGNO (new_reg), REGNO (original_reg));
+ REGNO (new_reg), original_regno);
ira_expand_reg_equiv ();
+
+ /* Copy the preference classes to new_reg. */
+ resize_reg_info ();
+ setup_reg_classes (REGNO (new_reg),
+ reg_preferred_class (original_regno),
+ reg_alternate_class (original_regno),
+ reg_allocno_class (original_regno));
+
return new_reg;
}
--
1.9.1
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2015-11-12 3:44 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-11-10 13:31 [PATCH] Fix IRA register preferencing Wilco Dijkstra
2015-11-12 3:44 ` Vladimir Makarov
-- strict thread matches above, loose matches on Subject: below --
2014-12-09 19:21 Wilco Dijkstra
2014-12-09 23:17 ` Jeff Law
2014-12-10 13:26 ` Wilco Dijkstra
2014-12-10 20:47 ` Jeff Law
2015-04-27 15:01 ` Wilco Dijkstra
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).