Patchwork [ARM,1/2] Make ARM_LEGITIMIZE_RELOAD_ADDRESS be a function

login
register
mail settings
Submitter Jie Zhang
Date Jan. 28, 2011, 4:46 p.m.
Message ID <4D42F2FC.6080306@codesourcery.com>
Download mbox | patch
Permalink /patch/80873/
State New
Headers show

Comments

Jie Zhang - Jan. 28, 2011, 4:46 p.m.
On 01/28/2011 10:26 PM, Richard Earnshaw wrote:
>
> On Wed, 2010-12-22 at 14:40 +0800, Jie Zhang wrote:
>> Hi,
>>
>> This patch should have no functionality changes. It just moves most of
>> ARM_LEGITIMIZE_RELOAD_ADDRESS from arm.h to
>> arm_legitimize_reload_address in arm.c. This is needed by the next
>> patch. It also eases debugging. Testing is going. OK if the result is good?
>>
>> Regards,
>
> So I think there's a subtle gotcha in this change that's easy to miss.
>
> +&&  REG_MODE_OK_FOR_BASE_P (XEXP (*p, 0), mode)
>
> is a macro that expands into
>
> #define REG_MODE_OK_FOR_BASE_P(X, MODE)         \
>    (TARGET_THUMB1                                \
>     ? THUMB1_REG_MODE_OK_FOR_BASE_P (X, MODE)    \
>     : ARM_REG_OK_FOR_BASE_P (X))
>
>
> But THUMB1_REG_MODE_OK_FOR_BASE_P and ARM_REG_OK_FOR_BASE_P both have
> two possible definitions, that are picked depending upon the file in
> which the macro is used (a nasty consequence of some attempt to save
> duplication of code a long long time ago in the early days of GCC).
>
> Now IIRC reload.c (which uses legitimize_reload_address) defines
> REG_OK_STRICT which leads to one definition of these macros, but arm.c
> does not (and nor should it), which leads to the other definition.
>
> Overall, I think that means that your patch has quietly changed the
> results that this macro can give.
>
Good catch! Thanks for review! How about expanding 
REG_MODE_OK_FOR_BASE_P a bit since we know when REG_MODE_OK_FOR_BASE_P 
is used in arm_legitimize_reload_address, REG_OK_STRICT is defined and
TARGET_THUMB1 is false?  In the attached patch, I replace

+      && REGNO (XEXP (*p, 0)) < FIRST_PSEUDO_REGISTER
+      && REG_MODE_OK_FOR_BASE_P (XEXP (*p, 0), mode)

with

+      && ARM_REGNO_OK_FOR_BASE_P (REGNO (XEXP (*p, 0)))


GCC build is OK. But I have not regression tested it yet.
Richard Earnshaw - Jan. 28, 2011, 5:03 p.m.
On Sat, 2011-01-29 at 00:46 +0800, Jie Zhang wrote:
> On 01/28/2011 10:26 PM, Richard Earnshaw wrote:
> >
> > On Wed, 2010-12-22 at 14:40 +0800, Jie Zhang wrote:
> >> Hi,
> >>
> >> This patch should have no functionality changes. It just moves most of
> >> ARM_LEGITIMIZE_RELOAD_ADDRESS from arm.h to
> >> arm_legitimize_reload_address in arm.c. This is needed by the next
> >> patch. It also eases debugging. Testing is going. OK if the result is good?
> >>
> >> Regards,
> >
> > So I think there's a subtle gotcha in this change that's easy to miss.
> >
> > +&&  REG_MODE_OK_FOR_BASE_P (XEXP (*p, 0), mode)
> >
> > is a macro that expands into
> >
> > #define REG_MODE_OK_FOR_BASE_P(X, MODE)         \
> >    (TARGET_THUMB1                                \
> >     ? THUMB1_REG_MODE_OK_FOR_BASE_P (X, MODE)    \
> >     : ARM_REG_OK_FOR_BASE_P (X))
> >
> >
> > But THUMB1_REG_MODE_OK_FOR_BASE_P and ARM_REG_OK_FOR_BASE_P both have
> > two possible definitions, that are picked depending upon the file in
> > which the macro is used (a nasty consequence of some attempt to save
> > duplication of code a long long time ago in the early days of GCC).
> >
> > Now IIRC reload.c (which uses legitimize_reload_address) defines
> > REG_OK_STRICT which leads to one definition of these macros, but arm.c
> > does not (and nor should it), which leads to the other definition.
> >
> > Overall, I think that means that your patch has quietly changed the
> > results that this macro can give.
> >
> Good catch! Thanks for review! How about expanding 
> REG_MODE_OK_FOR_BASE_P a bit since we know when REG_MODE_OK_FOR_BASE_P 
> is used in arm_legitimize_reload_address, REG_OK_STRICT is defined and
> TARGET_THUMB1 is false?  In the attached patch, I replace
> 
> +      && REGNO (XEXP (*p, 0)) < FIRST_PSEUDO_REGISTER
> +      && REG_MODE_OK_FOR_BASE_P (XEXP (*p, 0), mode)
> 
> with
> 
> +      && ARM_REGNO_OK_FOR_BASE_P (REGNO (XEXP (*p, 0)))
> 
> 
> GCC build is OK. But I have not regression tested it yet.
> 
> 

Yes, that sounds sensible.  Consider this approved once testing
completes.

Also, I notice that the same problem has crept into
thumb_legitimize_reload_address.  Perhaps you could correct that too in
a similar manner.  Consider such a patch pre-approved (but please commit
it separately from the ARM one).

R.
Jie Zhang - Jan. 29, 2011, 3:22 a.m.
On 01/29/2011 01:03 AM, Richard Earnshaw wrote:
>
> On Sat, 2011-01-29 at 00:46 +0800, Jie Zhang wrote:
>> On 01/28/2011 10:26 PM, Richard Earnshaw wrote:
>>>
>>> On Wed, 2010-12-22 at 14:40 +0800, Jie Zhang wrote:
>>>> Hi,
>>>>
>>>> This patch should have no functionality changes. It just moves most of
>>>> ARM_LEGITIMIZE_RELOAD_ADDRESS from arm.h to
>>>> arm_legitimize_reload_address in arm.c. This is needed by the next
>>>> patch. It also eases debugging. Testing is going. OK if the result is good?
>>>>
>>>> Regards,
>>>
>>> So I think there's a subtle gotcha in this change that's easy to miss.
>>>
>>> +&&   REG_MODE_OK_FOR_BASE_P (XEXP (*p, 0), mode)
>>>
>>> is a macro that expands into
>>>
>>> #define REG_MODE_OK_FOR_BASE_P(X, MODE)         \
>>>     (TARGET_THUMB1                                \
>>>      ? THUMB1_REG_MODE_OK_FOR_BASE_P (X, MODE)    \
>>>      : ARM_REG_OK_FOR_BASE_P (X))
>>>
>>>
>>> But THUMB1_REG_MODE_OK_FOR_BASE_P and ARM_REG_OK_FOR_BASE_P both have
>>> two possible definitions, that are picked depending upon the file in
>>> which the macro is used (a nasty consequence of some attempt to save
>>> duplication of code a long long time ago in the early days of GCC).
>>>
>>> Now IIRC reload.c (which uses legitimize_reload_address) defines
>>> REG_OK_STRICT which leads to one definition of these macros, but arm.c
>>> does not (and nor should it), which leads to the other definition.
>>>
>>> Overall, I think that means that your patch has quietly changed the
>>> results that this macro can give.
>>>
>> Good catch! Thanks for review! How about expanding
>> REG_MODE_OK_FOR_BASE_P a bit since we know when REG_MODE_OK_FOR_BASE_P
>> is used in arm_legitimize_reload_address, REG_OK_STRICT is defined and
>> TARGET_THUMB1 is false?  In the attached patch, I replace
>>
>> +&&  REGNO (XEXP (*p, 0))<  FIRST_PSEUDO_REGISTER
>> +&&  REG_MODE_OK_FOR_BASE_P (XEXP (*p, 0), mode)
>>
>> with
>>
>> +&&  ARM_REGNO_OK_FOR_BASE_P (REGNO (XEXP (*p, 0)))
>>
>>
>> GCC build is OK. But I have not regression tested it yet.
>>
>>
>
> Yes, that sounds sensible.  Consider this approved once testing
> completes.
>
Thanks! Tested with "-march=armv7-a -mfloat-abi=softfp -mfpu=neon" on 
qemu for gcc, g++ and libstdc++. No regressions. So I have committed the 
patch.

> Also, I notice that the same problem has crept into
> thumb_legitimize_reload_address.  Perhaps you could correct that too in
> a similar manner.  Consider such a patch pre-approved (but please commit
> it separately from the ARM one).
>
I will see if I can prepare a patch.

Regards,

Patch


	* config/arm/arm.c (arm_legitimize_reload_address): New.
	* config/arm/arm.h (ARM_LEGITIMIZE_RELOAD_ADDRESS): Use
	arm_legitimize_reload_address.
	* config/arm/arm-protos.h (arm_legitimize_reload_address):
	Declare.

Index: config/arm/arm.c
===================================================================
--- config/arm/arm.c	(revision 169362)
+++ config/arm/arm.c	(working copy)
@@ -6392,6 +6392,62 @@  thumb_legitimize_address (rtx x, rtx ori
   return x;
 }
 
+bool
+arm_legitimize_reload_address (rtx *p,
+			       enum machine_mode mode,
+			       int opnum, int type,
+			       int ind_levels ATTRIBUTE_UNUSED)
+{
+  if (GET_CODE (*p) == PLUS
+      && GET_CODE (XEXP (*p, 0)) == REG
+      && ARM_REGNO_OK_FOR_BASE_P (REGNO (XEXP (*p, 0)))
+      && GET_CODE (XEXP (*p, 1)) == CONST_INT)
+    {
+      HOST_WIDE_INT val = INTVAL (XEXP (*p, 1));
+      HOST_WIDE_INT low, high;
+
+      if (mode == DImode || (mode == DFmode && TARGET_SOFT_FLOAT))
+	low = ((val & 0xf) ^ 0x8) - 0x8;
+      else if (TARGET_MAVERICK && TARGET_HARD_FLOAT)
+	/* Need to be careful, -256 is not a valid offset.  */
+	low = val >= 0 ? (val & 0xff) : -((-val) & 0xff);
+      else if (mode == SImode
+	       || (mode == SFmode && TARGET_SOFT_FLOAT)
+	       || ((mode == HImode || mode == QImode) && ! arm_arch4))
+	/* Need to be careful, -4096 is not a valid offset.  */
+	low = val >= 0 ? (val & 0xfff) : -((-val) & 0xfff);
+      else if ((mode == HImode || mode == QImode) && arm_arch4)
+	/* Need to be careful, -256 is not a valid offset.  */
+	low = val >= 0 ? (val & 0xff) : -((-val) & 0xff);
+      else if (GET_MODE_CLASS (mode) == MODE_FLOAT
+	       && TARGET_HARD_FLOAT && TARGET_FPA)
+	/* Need to be careful, -1024 is not a valid offset.  */
+	low = val >= 0 ? (val & 0x3ff) : -((-val) & 0x3ff);
+      else
+	return false;
+
+      high = ((((val - low) & (unsigned HOST_WIDE_INT) 0xffffffff)
+	       ^ (unsigned HOST_WIDE_INT) 0x80000000)
+	      - (unsigned HOST_WIDE_INT) 0x80000000);
+      /* Check for overflow or zero */
+      if (low == 0 || high == 0 || (high + low != val))
+	return false;
+
+      /* Reload the high part into a base reg; leave the low part
+	 in the mem.  */
+      *p = gen_rtx_PLUS (GET_MODE (*p),
+			 gen_rtx_PLUS (GET_MODE (*p), XEXP (*p, 0),
+				       GEN_INT (high)),
+			 GEN_INT (low));
+      push_reload (XEXP (*p, 0), NULL_RTX, &XEXP (*p, 0), NULL,
+		   MODE_BASE_REG_CLASS (mode), GET_MODE (*p),
+		   VOIDmode, 0, 0, opnum, (enum reload_type) type);
+      return true;
+    }
+
+  return false;
+}
+
 rtx
 thumb_legitimize_reload_address (rtx *x_p,
 				 enum machine_mode mode,
Index: config/arm/arm.h
===================================================================
--- config/arm/arm.h	(revision 169362)
+++ config/arm/arm.h	(working copy)
@@ -1273,53 +1273,8 @@  enum reg_class
 #define ARM_LEGITIMIZE_RELOAD_ADDRESS(X, MODE, OPNUM, TYPE, IND, WIN)	   \
   do									   \
     {									   \
-      if (GET_CODE (X) == PLUS						   \
-	  && GET_CODE (XEXP (X, 0)) == REG				   \
-	  && REGNO (XEXP (X, 0)) < FIRST_PSEUDO_REGISTER		   \
-	  && REG_MODE_OK_FOR_BASE_P (XEXP (X, 0), MODE)			   \
-	  && GET_CODE (XEXP (X, 1)) == CONST_INT)			   \
-	{								   \
-	  HOST_WIDE_INT val = INTVAL (XEXP (X, 1));			   \
-	  HOST_WIDE_INT low, high;					   \
-									   \
-	  if (MODE == DImode || (MODE == DFmode && TARGET_SOFT_FLOAT))	   \
-	    low = ((val & 0xf) ^ 0x8) - 0x8;				   \
-	  else if (TARGET_MAVERICK && TARGET_HARD_FLOAT)		   \
-	    /* Need to be careful, -256 is not a valid offset.  */	   \
-	    low = val >= 0 ? (val & 0xff) : -((-val) & 0xff);		   \
-	  else if (MODE == SImode					   \
-		   || (MODE == SFmode && TARGET_SOFT_FLOAT)		   \
-		   || ((MODE == HImode || MODE == QImode) && ! arm_arch4)) \
-	    /* Need to be careful, -4096 is not a valid offset.  */	   \
-	    low = val >= 0 ? (val & 0xfff) : -((-val) & 0xfff);		   \
-	  else if ((MODE == HImode || MODE == QImode) && arm_arch4)	   \
-	    /* Need to be careful, -256 is not a valid offset.  */	   \
-	    low = val >= 0 ? (val & 0xff) : -((-val) & 0xff);		   \
-	  else if (GET_MODE_CLASS (MODE) == MODE_FLOAT			   \
-		   && TARGET_HARD_FLOAT && TARGET_FPA)			   \
-	    /* Need to be careful, -1024 is not a valid offset.  */	   \
-	    low = val >= 0 ? (val & 0x3ff) : -((-val) & 0x3ff);		   \
-	  else								   \
-	    break;							   \
-									   \
-	  high = ((((val - low) & (unsigned HOST_WIDE_INT) 0xffffffff)	   \
-		   ^ (unsigned HOST_WIDE_INT) 0x80000000)		   \
-		  - (unsigned HOST_WIDE_INT) 0x80000000);		   \
-	  /* Check for overflow or zero */				   \
-	  if (low == 0 || high == 0 || (high + low != val))		   \
-	    break;							   \
-									   \
-	  /* Reload the high part into a base reg; leave the low part	   \
-	     in the mem.  */						   \
-	  X = gen_rtx_PLUS (GET_MODE (X),				   \
-			    gen_rtx_PLUS (GET_MODE (X), XEXP (X, 0),	   \
-					  GEN_INT (high)),		   \
-			    GEN_INT (low));				   \
-	  push_reload (XEXP (X, 0), NULL_RTX, &XEXP (X, 0), NULL,	   \
-		       MODE_BASE_REG_CLASS (MODE), GET_MODE (X), 	   \
-		       VOIDmode, 0, 0, OPNUM, TYPE);			   \
-	  goto WIN;							   \
-	}								   \
+      if (arm_legitimize_reload_address (&X, MODE, OPNUM, TYPE, IND))	   \
+	goto WIN;							   \
     }									   \
   while (0)
 
Index: config/arm/arm-protos.h
===================================================================
--- config/arm/arm-protos.h	(revision 169362)
+++ config/arm/arm-protos.h	(working copy)
@@ -54,6 +54,8 @@  extern rtx legitimize_pic_address (rtx,
 extern rtx legitimize_tls_address (rtx, rtx);
 extern int arm_legitimate_address_outer_p (enum machine_mode, rtx, RTX_CODE, int);
 extern int thumb_legitimate_offset_p (enum machine_mode, HOST_WIDE_INT);
+extern bool arm_legitimize_reload_address (rtx *, enum machine_mode, int, int,
+					   int);
 extern rtx thumb_legitimize_reload_address (rtx *, enum machine_mode, int, int,
 					    int);
 extern int arm_const_double_rtx (rtx);