[patch committed] Defaulting to -fno-ira-share-spill-slots on SH

[patch committed] Defaulting to -fno-ira-share-spill-slots on SH

[Kaz Kojima]

Hi,

PR 37514 is a wrong-code problem on SH which is a 4.4 regression
started after the IRA merge.  IRA makes spill slots shared if
possible when flag_ira_share_spill_slots set.  In the above PR,
it causes a wrong sharing of slots on SH.  I've been trying to
see why this sharing occurs but it's a hard problem for me.
With -fno-ira-share-spill-slots, the problem goes away, though
it increases the code sizes of several tests in CSiBE by ~10%.
Although this code size regression isn't ignorable, I think
that the wrong code problem is far bigger than that.  I'd like
to work around this PR by defaulting to -fno-ira-share-spill-slots
on this target.  The attached patch is for this.  The patch also
adds a new target specific test to remember that the PR isn't fixed
yet.  It's tested with bootstrap and the top level "make -k check"
on sh4-unknown-linux-gnu.  The testsuite change is tested also
on i686-pc-linux-gnu.

Regards,
	kaz
--
[gcc/ChangeLog]
2008-11-10  Kaz Kojima  <kkojima@gcc.gnu.org>

	PR rtl-optimization/37514
	* config/sh/sh.h (OPTIMIZATION_OPTIONS): Set
	flag_ira_share_spill_slots to 2 if it's already non-zero.
	(OVERRIDE_OPTIONS): Clear flag_ira_share_spill_slots if
        flag_ira_share_spill_slots is 2.

[gcc/testsuite/ChangeLog]
2008-11-10  Kaz Kojima  <kkojima@gcc.gnu.org>

	* gcc.target/sh/pr37514.c: New test.

diff -uprN ORIG/trunk/gcc/config/sh/sh.h LOCAL/trunk/gcc/config/sh/sh.h
--- ORIG/trunk/gcc/config/sh/sh.h	2008-10-22 09:11:15.000000000 +0900
+++ LOCAL/trunk/gcc/config/sh/sh.h	2008-11-08 07:59:02.000000000 +0900
@@ -496,6 +496,9 @@ do {									\
      the user explicitly requested this to be on or off.  */		\
   if (flag_schedule_insns > 0)						\
     flag_schedule_insns = 2;						\
+  /* Likewise for flag_ira_share_spill_slots.  */			\
+  if (flag_ira_share_spill_slots > 0)					\
+    flag_ira_share_spill_slots = 2;					\
 									\
   set_param_value ("simultaneous-prefetches", 2);			\
 } while (0)
@@ -730,6 +733,12 @@ do {									\
 	}								\
     }									\
 									\
+  /* FIXME.  Currently -fira-share-spill-slots causes a wrong code	\
+     problem PR 37514, though the compiler generates lengthy codes	\
+     in some cases without it.  */					\
+  if (flag_ira_share_spill_slots == 2)					\
+    flag_ira_share_spill_slots = 0;					\
+									\
   if (align_loops == 0)							\
     align_loops =  1 << (TARGET_SH5 ? 3 : 2);				\
   if (align_jumps == 0)							\
diff -uprN ORIG/trunk/gcc/testsuite/gcc.target/sh/pr37514.c LOCAL/trunk/gcc/testsuite/gcc.target/sh/pr37514.c
--- ORIG/trunk/gcc/testsuite/gcc.target/sh/pr37514.c	1970-01-01 09:00:00.000000000 +0900
+++ LOCAL/trunk/gcc/testsuite/gcc.target/sh/pr37514.c	2008-11-08 08:02:27.000000000 +0900
@@ -0,0 +1,65 @@
+/* This is essentially gcc.c-torture/execute/20021120-1.c run with
+   -O3 -fomit-frame-pointer -fira-share-spill-slots.  */
+/* { dg-do run { target "sh*-*-*" } } */
+/* { dg-options "-O3 -fomit-frame-pointer -fira-share-spill-slots" } */
+
+/* Macros to emit "L Nxx R" for each octal number xx between 000 and 037.  */
+#define OP1(L, N, R, I, J) L N##I##J R
+#define OP2(L, N, R, I) \
+    OP1(L, N, R, 0, I), OP1(L, N, R, 1, I), \
+    OP1(L, N, R, 2, I), OP1(L, N, R, 3, I)
+#define OP(L, N, R) \
+    OP2(L, N, R, 0), OP2(L, N, R, 1), OP2(L, N, R, 2), OP2(L, N, R, 3), \
+    OP2(L, N, R, 4), OP2(L, N, R, 5), OP2(L, N, R, 6), OP2(L, N, R, 7)
+
+/* Declare 32 unique variables with prefix N.  */
+#define DECLARE(N) OP (, N,)
+
+/* Copy 32 variables with prefix N from the array at ADDR.
+   Leave ADDR pointing to the end of the array.  */
+#define COPYIN(N, ADDR) OP (, N, = *(ADDR++))
+
+/* Likewise, but copy the other way.  */
+#define COPYOUT(N, ADDR) OP (*(ADDR++) =, N,)
+
+/* Add the contents of the array at ADDR to 32 variables with prefix N.
+   Leave ADDR pointing to the end of the array.  */
+#define ADD(N, ADDR) OP (, N, += *(ADDR++))
+
+volatile double gd[32];
+volatile float gf[32];
+
+extern void abort (void);
+
+static void foo (int n)
+{
+  double DECLARE(d);
+  float DECLARE(f);
+  volatile double *pd;
+  volatile float *pf;
+  int i;
+
+  pd = gd; COPYIN (d, pd);
+  for (i = 0; i < n; i++)
+    {
+      pf = gf; COPYIN (f, pf);
+      pd = gd; ADD (d, pd);
+      pd = gd; ADD (d, pd);
+      pd = gd; ADD (d, pd);
+      pf = gf; COPYOUT (f, pf);
+    }
+  pd = gd; COPYOUT (d, pd);
+}
+
+int main ()
+{
+  int i;
+
+  for (i = 0; i < 32; i++)
+    gd[i] = i, gf[i] = i;
+  foo (1);
+  for (i = 0; i < 32; i++)
+    if (gd[i] != i * 4 || gf[i] != i)
+      abort ();
+  return 0;
+}

Re: [patch committed] Defaulting to -fno-ira-share-spill-slots on SH

[Bernd Schmidt]

Kaz Kojima wrote:
> PR 37514 is a wrong-code problem on SH which is a 4.4 regression
> started after the IRA merge.

> With -fno-ira-share-spill-slots, the problem goes away, though
> it increases the code sizes of several tests in CSiBE by ~10%.
> Although this code size regression isn't ignorable, I think
> that the wrong code problem is far bigger than that.  I'd like
> to work around this PR by defaulting to -fno-ira-share-spill-slots
> on this target.

Wouldn't it be better to default to the old register allocator in this
case?  Or simply leave it since hopefully 4.4 won't be released with
this kind of issue in the register allocator.


Bernd
-- 
This footer brought to you by insane German lawmakers.
Analog Devices GmbH      Wilhelm-Wagenfeld-Str. 6      80807 Muenchen
Sitz der Gesellschaft Muenchen, Registergericht Muenchen HRB 40368
Geschaeftsfuehrer Thomas Wessel, William A. Martin, Margaret Seif

Re: [patch committed] Defaulting to -fno-ira-share-spill-slots on SH

[Kaz Kojima]

Bernd Schmidt <bernds_cb1@t-online.de> wrote:
> Wouldn't it be better to default to the old register allocator in this
> case?  Or simply leave it since hopefully 4.4 won't be released with
> this kind of issue in the register allocator.

It's surely better and would be the right way if this was
the problem for the primary or the secondary targets.
My work-around patch is to avoid releasing 4.4 with this
wrong code issue on SH without preventing the other targets
moving to the new register allocator from the old allocator
which is planed to be removed in the near future.  Vlad kindly
says in a trail of the PR that he will take a look at this
issue, so I hope that this temporary work-around can be
reverted and the PR will get the right fix before getting
4.4 out of the door.

Regards,
	kaz

Re: [patch committed] Defaulting to -fno-ira-share-spill-slots on SH

[Jeff Law]

Kaz Kojima wrote:
> Hi,
>
> PR 37514 is a wrong-code problem on SH which is a 4.4 regression
> started after the IRA merge.  IRA makes spill slots shared if
> possible when flag_ira_share_spill_slots set.  In the above PR,
> it causes a wrong sharing of slots on SH.  I've been trying to
> see why this sharing occurs but it's a hard problem for me.
> With -fno-ira-share-spill-slots, the problem goes away, though
> it increases the code sizes of several tests in CSiBE by ~10%.
> Although this code size regression isn't ignorable, I think
> that the wrong code problem is far bigger than that.  I'd like
> to work around this PR by defaulting to -fno-ira-share-spill-slots
> on this target.  The attached patch is for this.  The patch also
> adds a new target specific test to remember that the PR isn't fixed
> yet.  It's tested with bootstrap and the top level "make -k check"
> on sh4-unknown-linux-gnu.  The testsuite change is tested also
> on i686-pc-linux-gnu.
>
> Regards,
> 	kaz
> --
> [gcc/ChangeLog]
> 2008-11-10  Kaz Kojima  <kkojima@gcc.gnu.org>
>
> 	PR rtl-optimization/37514
> 	* config/sh/sh.h (OPTIMIZATION_OPTIONS): Set
> 	flag_ira_share_spill_slots to 2 if it's already non-zero.
> 	(OVERRIDE_OPTIONS): Clear flag_ira_share_spill_slots if
>         flag_ira_share_spill_slots is 2.
>   
Seems like absolutely the wrong approach to me.   I'm surprised we're 
still having stack slot sharing problems with the patch for 37447 installed.

jeff

Re: [patch committed] Defaulting to -fno-ira-share-spill-slots on SH

[Kaz Kojima]

Jeff Law <law@redhat.com> wrote:
> Seems like absolutely the wrong approach to me.   I'm surprised we're 
> still having stack slot sharing problems with the patch for 37447 installed.

Vlad has pointed out that my approach can't work stably:
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=37514#c6

and, in fact, the recent changes of compiler makes 20021120-1.c
failed even with -fno-ira-share-spill-slots.  I'll revert
my patch.

Regards,
	kaz