diff mbox

[SH] PR target/56995

Message ID 516FBB25.8080103@st.com
State New
Headers show

Commit Message

Christian Bruel April 18, 2013, 9:21 a.m. UTC
Hello,

While checking the register classes definitions to fix this ICE, I
noticed that DF_HI_REGS seems to be always strictly equivalent to DF_REGS...

Indeed, we have:

/* DF_HI_REGS: Initialized TARGET_CONDITIONAL_REGISTER_USAGE.*/		
  { 0x00000000, 0x00000000, 0xffffffff, 0xffffffff, 0x0000ff00 },	
/* DF_REGS:  */								
  { 0x00000000, 0x00000000, 0xffffffff, 0xffffffff, 0x0000ff00 },

and with sh_conditional_register_usage

  for (regno = FIRST_FP_REG + (TARGET_LITTLE_ENDIAN != 0);
       regno <= LAST_FP_REG; regno += 2)
    SET_HARD_REG_BIT (reg_class_contents[DF_HI_REGS], regno);

but the FP_REGS regno are already set ...

So, Just removing DF_HI_REGS seems to fix the issue with strictly same
performance results for SH4.

  No regressions in the testsuite for
    sh-sim//-m2/
    sh-sim//-m2a/
    sh-sim//-m2a-nofpu/
    sh-sim//-m2a-single/
    sh-sim//-m2a-single-only/
    sh-sim//-m3/
    sh-sim//-m3e/
    sh-sim//-m4/
    sh-sim//-m4-single/
    sh-sim//-m4-single-only/
    sh-sim//-m4a/
    sh-sim//-m4a-single/
    sh-sim//-m4a-single-only/

 *[-mb,-ml]

 No performance regression for -m4

Hoping that I haven't missed something totally obvious with this class
duplication... I'll be glad to have your feedback.

The consequence of this it that find_costs_and_classes seems to be
confused when two register classes are strictly equivalent. Is it
plausible ?

note that experimentally, I tried to reset the DF_HI_REGS class
definition so it gets only the even registers set with
sh_conditional_register_usage. This is also functional but gives very
small worse code generation.

I also simplified the mfmovd.c test to check for hard_float.

Thanks a lot any other comments.

Christian

Comments

Kaz Kojima April 18, 2013, 2:27 p.m. UTC | #1
Christian Bruel <christian.bruel@st.com> wrote:
> So, Just removing DF_HI_REGS seems to fix the issue with strictly same
> performance results for SH4.
> 
>   No regressions in the testsuite for
>     sh-sim//-m2/
>     sh-sim//-m2a/
>     sh-sim//-m2a-nofpu/
>     sh-sim//-m2a-single/
>     sh-sim//-m2a-single-only/
>     sh-sim//-m3/
>     sh-sim//-m3e/
>     sh-sim//-m4/
>     sh-sim//-m4-single/
>     sh-sim//-m4-single-only/
>     sh-sim//-m4a/
>     sh-sim//-m4a-single/
>     sh-sim//-m4a-single-only/
> 
>  *[-mb,-ml]
> 
>  No performance regression for -m4

The patch is OK.
It seems that your patch does the right thing, though I don't
know the history of DF_HI_REGS at all.

> The consequence of this it that find_costs_and_classes seems to be
> confused when two register classes are strictly equivalent. Is it
> plausible ?

Looks very likely to me.

Regards,
	kaz
diff mbox

Patch

2013-04-18  Christian Bruel  <christian.bruel@st.com>

	PR target/56995
	* gcc.target/sh/mfmovd.c: Add new function and check hard_float.

2013-04-18  Christian Bruel  <christian.bruel@st.com>

	PR target/56995
	* config/sh/sh.h (enum reg_class): Remove DF_HI_REGS.
	(REG_CLASS_NAMES): Idem.
	(REG_CLASS_CONTENTS): Idem.
	(REGCLASS_HAS_FP_REG): Idem.
	* config/sh/sh.c (sh_cannot_change_mode_class): Idem.
	(sh_conditional_register_usage): Idem.

Index: gcc/testsuite/gcc.target/sh/mfmovd.c
===================================================================
--- gcc/testsuite/gcc.target/sh/mfmovd.c	(revision 197895)
+++ gcc/testsuite/gcc.target/sh/mfmovd.c	(working copy)
@@ -1,8 +1,9 @@ 
 /* Verify that we generate fmov.d instructions to move doubles when -mfmovd 
    option is enabled.  */
 /* { dg-do compile { target "sh*-*-*" } } */
+/* { dg-require-effective-target hard_float } */
 /* { dg-options "-mfmovd" } */
-/* { dg-skip-if "" { "sh*-*-*" } { "*" } { "-m2a" "-m2a-single" "-m4" "-m4-single" "-m4-100" "-m4-100-single" "-m4-200" "-m4-200-single" "-m4-300" "-m4-300-single" "-m4a" "-m4a-single" } }  */
+/* { dg-skip-if "" { "*-single-only" } { "" } }  */
 /* { dg-final { scan-assembler "fmov.d" } } */
 
 extern double g;
@@ -13,3 +14,9 @@  f (double d)
   g = d;
 }
 
+extern float h;
+
+void f2 ()
+{
+  h = g;
+}
Index: gcc/config/sh/sh.c
===================================================================
--- gcc/config/sh/sh.c	(revision 197895)
+++ gcc/config/sh/sh.c	(working copy)
@@ -12163,7 +12163,7 @@  sh_cannot_change_mode_class (enum machine_mode fro
       else
 	{
 	  if (GET_MODE_SIZE (from) < 8)
-	    return reg_classes_intersect_p (DF_HI_REGS, rclass);
+	    return reg_classes_intersect_p (DF_REGS, rclass);
 	}
     }
   return false;
@@ -13210,9 +13210,7 @@  sh_conditional_register_usage (void)
       call_really_used_regs[MACH_REG] = 0;
       call_really_used_regs[MACL_REG] = 0;
     }
-  for (regno = FIRST_FP_REG + (TARGET_LITTLE_ENDIAN != 0);
-       regno <= LAST_FP_REG; regno += 2)
-    SET_HARD_REG_BIT (reg_class_contents[DF_HI_REGS], regno);
+
   if (TARGET_SHMEDIA)
     {
       for (regno = FIRST_TARGET_REG; regno <= LAST_TARGET_REG; regno ++)
Index: gcc/config/sh/sh.h
===================================================================
--- gcc/config/sh/sh.h	(revision 197895)
+++ gcc/config/sh/sh.h	(working copy)
@@ -984,7 +984,6 @@  enum reg_class
   GENERAL_REGS,
   FP0_REGS,
   FP_REGS,
-  DF_HI_REGS,
   DF_REGS,
   FPSCR_REGS,
   GENERAL_FP_REGS,
@@ -1010,7 +1009,6 @@  enum reg_class
   "GENERAL_REGS",	\
   "FP0_REGS",		\
   "FP_REGS",		\
-  "DF_HI_REGS",		\
   "DF_REGS",		\
   "FPSCR_REGS",		\
   "GENERAL_FP_REGS",	\
@@ -1046,8 +1044,6 @@  enum reg_class
   { 0x00000000, 0x00000000, 0x00000001, 0x00000000, 0x00000000 },	\
 /* FP_REGS:  */								\
   { 0x00000000, 0x00000000, 0xffffffff, 0xffffffff, 0x00000000 },	\
-/* DF_HI_REGS:  Initialized in TARGET_CONDITIONAL_REGISTER_USAGE.  */	\
-  { 0x00000000, 0x00000000, 0xffffffff, 0xffffffff, 0x0000ff00 },	\
 /* DF_REGS:  */								\
   { 0x00000000, 0x00000000, 0xffffffff, 0xffffffff, 0x0000ff00 },	\
 /* FPSCR_REGS:  */							\
@@ -1922,7 +1918,7 @@  struct sh_args {
 
 #define REGCLASS_HAS_FP_REG(CLASS) \
   ((CLASS) == FP0_REGS || (CLASS) == FP_REGS \
-   || (CLASS) == DF_REGS || (CLASS) == DF_HI_REGS)
+   || (CLASS) == DF_REGS)
 
 /* ??? Perhaps make MEMORY_MOVE_COST depend on compiler option?  This
    would be so that people with slow memory systems could generate