diff mbox

Fix PR45051

Message ID 4C4EA9C3.8080405@codesourcery.com
State New
Headers show

Commit Message

Bernd Schmidt July 27, 2010, 9:41 a.m. UTC 
The IRA patch last week exposed a reload problem on cris.  For some
reason the port seems to be generating too many reloads when subregs of
DImode values are involved, but that's something for the port
maintainers to look at.  It exposed a problem in delete_output_reload
where we have

(set (reg:SI 0) (...))  ; insn, r0 is the reload register
(set (reg:SI 9) (reg:SI 0)) ; output reload for (subreg (reg x) 4)
(set (mem:DI z) (reg 8)) ; DImode reg x got hard reg 8.
...
(set (something) (reg:SI 0)) ; inheriting reg 0 for reloading part of X
  (reg_dead x)

Here, we delete the output reload insn, because we can't see that the
store to memory uses reg 9: reg_mentioned_p doesn't take hard regs with
multiple words into account.

Fixed with this patch; bootstrapped and regression tested on i686-linux
and committed.


Bernd
PR rtl-optimization/45051
	* reload1.c (delete_output_reload): Use refers_to_regno_p rather
	than reg_mentioned_p.

Comments

Hans-Peter Nilsson July 28, 2010, 1:46 a.m. UTC | #1 
On Tue, 27 Jul 2010, Bernd Schmidt wrote:
> The IRA patch last week exposed a reload problem on cris.

Thanks for fixing the bug promptly.

>  For some
> reason the port seems to be generating too many reloads when subregs of
> DImode values are involved, but that's something for the port
> maintainers to look at.

Thanks, yes.  I assume you mean for the reduced abs-2.c and r162418?

I see 13 reloads ("./xgcc -B./ -S -O2 -da abs-2.i; grep '^Reload ' *.ira"):
Reload 0: reload_out (DI) = (mem/v/c/i:DI (plus:SI (reg/f:SI 14 sp)
Reload 0: reload_out (DI) = (mem/v/c/i:DI (plus:SI (reg/f:SI 14 sp)
Reload 0: reload_out (DI) = (mem/v/c/i:DI (plus:SI (reg/f:SI 14 sp)
Reload 0: reload_out (SI) = (reg:SI 8 r8)
Reload 0: reload_out (SI) = (reg:SI 9 r9)
Reload 0: reload_out (SI) = (reg:SI 8 r8)
Reload 0: reload_out (SI) = (reg:SI 9 r9)
Reload 0: reload_out (DI) = (mem/v/c/i:DI (plus:SI (reg/f:SI 14 sp)
Reload 0: reload_out (DI) = (mem/v/c/i:DI (plus:SI (reg/f:SI 14 sp)
Reload 0: reload_in (SI) = (reg:SI 9 r9)
Reload 0: reload_out (DI) = (mem/v/c/i:DI (plus:SI (reg/f:SI 14 sp)
Reload 0: reload_in (DI) = (mem/v/c/i:DI (plus:SI (reg/f:SI 14 sp)
Reload 0: reload_in (DI) = (reg:DI 6 r6 [34])

The mem ones are expected, for fp+offset1 -> sp+offset2.

The r6 one is for *subdi3_non_v32 where IRA didn't succeed for
constraints "&r / "0" / "g"; yielding the reload-requiring
r12 / r9 / r12 given (reg:DI 33) / (reg:DI 34) / (reg:DI 33).
Perhaps a deficiency, I'm not sure if to expect that much from
IRA.

The SI r8/r9 ones (subreg, right) are arguably fishy, probably
related to FRAME_POINTER_REGNUM = r8 (i.e. using an eliminable
non-fixed register like m68k, not using a fake virtual register
like most other ports).  I believe IRA (or at least Vlad)
doesn't like that; see
<http://gcc.gnu.org/bugzilla/show_bug.cgi?id=41085#c2>.

If you'd care to elaborate or confirm that your observation was
along those lines, it'd help.  Thanks.

brgds, H-P
Bernd Schmidt July 28, 2010, 2:11 a.m. UTC | #2 
On 07/28/2010 03:46 AM, Hans-Peter Nilsson wrote:
> The SI r8/r9 ones (subreg, right) are arguably fishy, probably
> related to FRAME_POINTER_REGNUM = r8 (i.e. using an eliminable
> non-fixed register like m68k, not using a fake virtual register
> like most other ports).  I believe IRA (or at least Vlad)
> doesn't like that; see
> <http://gcc.gnu.org/bugzilla/show_bug.cgi?id=41085#c2>.
> 
> If you'd care to elaborate or confirm that your observation was
> along those lines, it'd help.  Thanks.

Huh, interesting.  Not sure about whether using a real reg as
FRAME_POINTER_REGNUM is a problem, but I'd change it since eliminating a
dummy reg into the hard frame pointer is pretty much standard practice
these days and IMO somewhat cleaner.  Also, have a look at

http://gcc.gnu.org/ml/gcc-patches/2010-07/msg00756.html

The patch won't do anything on your port due to the FRAME_POINTER_REGNUM
definition, but you could try deleting the entire if statement to see if
that makes the unnecessary reloads go away.


Bernd
Hans-Peter Nilsson July 28, 2010, 4:24 a.m. UTC | #3 
On Wed, 28 Jul 2010, Bernd Schmidt wrote:
> On 07/28/2010 03:46 AM, Hans-Peter Nilsson wrote:
> > The SI r8/r9 ones (subreg, right) are arguably fishy, probably
> > related to FRAME_POINTER_REGNUM = r8 (i.e. using an eliminable
> > non-fixed register like m68k, not using a fake virtual register
> > like most other ports).  I believe IRA (or at least Vlad)
> > doesn't like that; see
> > <http://gcc.gnu.org/bugzilla/show_bug.cgi?id=41085#c2>.
> >
> > If you'd care to elaborate or confirm that your observation was
> > along those lines, it'd help.  Thanks.
>
> Huh, interesting.  Not sure about whether using a real reg as
> FRAME_POINTER_REGNUM is a problem, but I'd change it since eliminating a
> dummy reg into the hard frame pointer is pretty much standard practice
> these days and IMO somewhat cleaner.

IMO still not much cleanliness in each port doing that, it could
and should be handled by the generic parts of gcc, that and
similarly ARG_POINTER_REGNUM. ...but no, no such generic patch
forthcoming. :)

I do plan to "do what [almost] everybody else does" (it's not
that many lines for one), just want to have PR41085 fixed before
covering it up finally.  The below would answer any remaining
"does it actually pessimize code" question, assuming your patch
goes in unmodified.

> Also, have a look at
>
> http://gcc.gnu.org/ml/gcc-patches/2010-07/msg00756.html

Wow, right on spot.

> The patch won't do anything on your port due to the FRAME_POINTER_REGNUM
> definition, but you could try deleting the entire if statement to see if
> that makes the unnecessary reloads go away.

As expected, it does make them go away.  Is there any point in
that if-statement, given the REG_CANNOT_CHANGE_MODE_P guard in
the if-statement above it?

Thanks.

brgds, H-P
Bernd Schmidt July 28, 2010, 9:50 a.m. UTC | #4 
On 07/28/2010 06:24 AM, Hans-Peter Nilsson wrote:
> On Wed, 28 Jul 2010, Bernd Schmidt wrote:
>> Also, have a look at
>>
>> http://gcc.gnu.org/ml/gcc-patches/2010-07/msg00756.html
> 
> Wow, right on spot.
> 
>> The patch won't do anything on your port due to the FRAME_POINTER_REGNUM
>> definition, but you could try deleting the entire if statement to see if
>> that makes the unnecessary reloads go away.
> 
> As expected, it does make them go away.  Is there any point in
> that if-statement, given the REG_CANNOT_CHANGE_MODE_P guard in
> the if-statement above it?

I don't think there is, but the patch has been unreviewed for a while...


Bernd
diff mbox

Patch

Index: reload1.c
===================================================================
--- reload1.c	(revision 162421)
+++ reload1.c	(working copy)
@@ -8813,6 +8813,8 @@  delete_output_reload (rtx insn, int j, i
   int n_inherited = 0;
   rtx i1;
   rtx substed;
+  unsigned regno;
+  int nregs;
 
   /* It is possible that this reload has been only used to set another reload
      we eliminated earlier and thus deleted this instruction too.  */
@@ -8864,6 +8866,12 @@  delete_output_reload (rtx insn, int j, i
   if (n_occurrences > n_inherited)
     return;
 
+  regno = REGNO (reg);
+  if (regno >= FIRST_PSEUDO_REGISTER)
+    nregs = 1;
+  else
+    nregs = hard_regno_nregs[regno][GET_MODE (reg)];
+
   /* If the pseudo-reg we are reloading is no longer referenced
      anywhere between the store into it and here,
      and we're within the same basic block, then the value can only
@@ -8875,7 +8883,7 @@  delete_output_reload (rtx insn, int j, i
       if (NOTE_INSN_BASIC_BLOCK_P (i1))
 	return;
       if ((NONJUMP_INSN_P (i1) || CALL_P (i1))
-	  && reg_mentioned_p (reg, PATTERN (i1)))
+	  && refers_to_regno_p (regno, regno + nregs, PATTERN (i1), NULL))
 	{
 	  /* If this is USE in front of INSN, we only have to check that
 	     there are no more references than accounted for by inheritance.  */