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

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

[Jie Zhang]

[-- Attachment #1: Type: text/plain, Size: 292 bytes --]

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,
-- 
Jie Zhang



[-- Attachment #2: gcc-arm-functionize-legitimize_reload_address-3.diff --]
[-- Type: text/x-diff, Size: 6048 bytes --]


	* 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.

diff --git a/gcc/config/arm/arm-protos.h b/gcc/config/arm/arm-protos.h
index 53923bd..f037a45 100644
--- a/gcc/config/arm/arm-protos.h
+++ b/gcc/config/arm/arm-protos.h
@@ -54,6 +54,8 @@ extern rtx legitimize_pic_address (rtx, enum machine_mode, 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);
diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
index 2aaec8c..95bde4a 100644
--- a/gcc/config/arm/arm.c
+++ b/gcc/config/arm/arm.c
@@ -6388,6 +6388,63 @@ thumb_legitimize_address (rtx x, rtx orig_x, enum machine_mode mode)
   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
+      && REGNO (XEXP (*p, 0)) < FIRST_PSEUDO_REGISTER
+      && REG_MODE_OK_FOR_BASE_P (XEXP (*p, 0), mode)
+      && 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,
diff --git a/gcc/config/arm/arm.h b/gcc/config/arm/arm.h
index 9951553..6abc832 100644
--- a/gcc/config/arm/arm.h
+++ b/gcc/config/arm/arm.h
@@ -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)
 

PING [ARM] [1/2] Make ARM_LEGITIMIZE_RELOAD_ADDRESS be a function

[Jie Zhang]

PING

On 12/22/2010 02:40 PM, 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,


-- 
Jie Zhang

Re: [ARM] [1/2] Make ARM_LEGITIMIZE_RELOAD_ADDRESS be a function

[Ramana Radhakrishnan]

Hi Jie,

On Wed, Dec 22, 2010 at 6:40 AM, Jie Zhang <jie@codesourcery.com>
wrote:Testing is
> going. OK if the result is good?

I can't approve or reject your patch though personally I think this is
a good change. I've eyeballed it once and it looks good. Hopefully
your testing showed no regressions :) ?


Ramana

Re: [ARM] [1/2] Make ARM_LEGITIMIZE_RELOAD_ADDRESS be a function

[Jie Zhang]

On 01/27/2011 09:39 AM, Ramana Radhakrishnan wrote:
> Hi Jie,
>
> On Wed, Dec 22, 2010 at 6:40 AM, Jie Zhang<jie@codesourcery.com>
> wrote:Testing is
>> going. OK if the result is good?
>
> I can't approve or reject your patch though personally I think this is
> a good change. I've eyeballed it once and it looks good. Hopefully
> your testing showed no regressions :) ?
>
Thanks for your review. I confirm that my testing shows no regressions.

Regards,
-- 
Jie Zhang

Re: [ARM] [1/2] Make ARM_LEGITIMIZE_RELOAD_ADDRESS be a function

[Richard Earnshaw]

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.

R.

Re: [ARM] [1/2] Make ARM_LEGITIMIZE_RELOAD_ADDRESS be a function

[Jie Zhang]

[-- Attachment #1: Type: text/plain, Size: 1868 bytes --]

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.


-- 
Jie Zhang


[-- Attachment #2: gcc-arm-functionize-legitimize_reload_address-4.diff --]
[-- Type: text/x-diff, Size: 6013 bytes --]


	* 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);

Re: [ARM] [1/2] Make ARM_LEGITIMIZE_RELOAD_ADDRESS be a function

[Richard Earnshaw]

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.

Re: [ARM] [1/2] Make ARM_LEGITIMIZE_RELOAD_ADDRESS be a function

[Jie Zhang]

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,
-- 
Jie Zhang

Re: [ARM] [1/2] Make ARM_LEGITIMIZE_RELOAD_ADDRESS be a function

[Jie Zhang]

[-- Attachment #1: Type: text/plain, Size: 1561 bytes --]

On 01/29/2011 11:22 AM, Jie Zhang wrote:
> On 01/29/2011 01:03 AM, Richard Earnshaw wrote:
>> 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.
>
I take a look. Yeah, there are some issues in 
thumb_legitimize_reload_address.

1. thumb_legitimize_reload_address uses REG_MODE_OK_FOR_REG_BASE_P, 
which will use an undefined macro ARM_REGNO_OK_FOR_INDEX_P if 
REG_OK_STRICT was defined. But since thumb_legitimize_reload_address is 
defined in arm.c, in which REG_OK_STRICT is not defined, this does not 
cause a compile time bug now.

2. I tried to use REGNO_OK_FOR_INDEX_P instead of 
ARM_REGNO_OK_FOR_INDEX_P and apply the attached patch. GCC would have an 
ICE on the attached test case, which is reduced from a newlib source 
file, with options "-O2 -mthumb".

3. Currently we use thumb_legitimize_reload_address to handle 
TARGET_THUMB1 and TARGET_THUMB2. It seems not good. I think we may need 
to use separate functions for each case.

4. The code piece which uses REG_MODE_OK_FOR_REG_BASE_P in 
thumb_legitimize_reload_address was added to fix PR target/23436. That 
bug cannot be reproduced on the latest GCC trunk even after commenting 
out that code piece. Maybe we don't need that code piece now?

So it seems there is no simple fix for this problem. But I may have not 
much time for this now.


Regards,
-- 
Jie Zhang

[-- Attachment #2: t.c --]
[-- Type: text/x-csrc, Size: 809 bytes --]

typedef unsigned long __ULong;
struct _on_exit_args
{
  void *_dso_handle[32];
  __ULong _fntypes;
  __ULong _is_cxa;
};
struct _atexit
{
  int _ind;
  struct _on_exit_args _on_exit_args;
};
struct _reent
{
  int _errno;
  union
  {
    struct
    {
      unsigned int _unused_rand;
      unsigned int _nmalloc[30];
    }
  }
  _new;
  struct _atexit *_atexit;
};
extern struct _reent *const _global_impure_ptr;
__call_exitprocs (void *d)
{
  register struct _atexit *p;
  register struct _on_exit_args *args;
  register int n;
  int i;
restart:
  p = _global_impure_ptr->_atexit;
  {
    args = &p->_on_exit_args;
    for (;;)
      {
	i = 1 << n;
	if (d && (!args || args->_dso_handle[n] != d))
	  if ((args->_fntypes & i) == 0)
	    fn ();
	  else if ((args->_is_cxa & i))
	    goto restart;
      }
  }
}

[-- Attachment #3: ttt-gcc-arm-fix-thumb-legitimize-address-2.diff --]
[-- Type: text/x-diff, Size: 576 bytes --]

Index: config/arm/arm.c
===================================================================
--- config/arm/arm.c	(revision 169386)
+++ config/arm/arm.c	(working copy)
@@ -6477,8 +6477,8 @@ thumb_legitimize_reload_address (rtx *x_
   if (GET_CODE (x) == PLUS
       && REG_P (XEXP (x, 0))
       && REG_P (XEXP (x, 1))
-      && !REG_MODE_OK_FOR_REG_BASE_P (XEXP (x, 0), mode)
-      && !REG_MODE_OK_FOR_REG_BASE_P (XEXP (x, 1), mode))
+      && !REGNO_OK_FOR_INDEX_P (REGNO (XEXP (x, 0)))
+      && !REGNO_OK_FOR_INDEX_P (REGNO (XEXP (x, 1))))
     {
       rtx orig_x = x;
 

Re: [ARM] [1/2] Make ARM_LEGITIMIZE_RELOAD_ADDRESS be a function

[Jie Zhang]

[-- Attachment #1: Type: text/plain, Size: 1561 bytes --]

On 01/29/2011 11:22 AM, Jie Zhang wrote:
> On 01/29/2011 01:03 AM, Richard Earnshaw wrote:
>> 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.
>
I take a look. Yeah, there are some issues in 
thumb_legitimize_reload_address.

1. thumb_legitimize_reload_address uses REG_MODE_OK_FOR_REG_BASE_P, 
which will use an undefined macro ARM_REGNO_OK_FOR_INDEX_P if 
REG_OK_STRICT was defined. But since thumb_legitimize_reload_address is 
defined in arm.c, in which REG_OK_STRICT is not defined, this does not 
cause a compile time bug now.

2. I tried to use REGNO_OK_FOR_INDEX_P instead of 
ARM_REGNO_OK_FOR_INDEX_P and apply the attached patch. GCC would have an 
ICE on the attached test case, which is reduced from a newlib source 
file, with options "-O2 -mthumb".

3. Currently we use thumb_legitimize_reload_address to handle 
TARGET_THUMB1 and TARGET_THUMB2. It seems not good. I think we may need 
to use separate functions for each case.

4. The code piece which uses REG_MODE_OK_FOR_REG_BASE_P in 
thumb_legitimize_reload_address was added to fix PR target/23436. That 
bug cannot be reproduced on the latest GCC trunk even after commenting 
out that code piece. Maybe we don't need that code piece now?

So it seems there is no simple fix for this problem. But I may have not 
much time for this now.


Regards,
-- 
Jie Zhang

[-- Attachment #2: t.c --]
[-- Type: text/x-csrc, Size: 809 bytes --]

typedef unsigned long __ULong;
struct _on_exit_args
{
  void *_dso_handle[32];
  __ULong _fntypes;
  __ULong _is_cxa;
};
struct _atexit
{
  int _ind;
  struct _on_exit_args _on_exit_args;
};
struct _reent
{
  int _errno;
  union
  {
    struct
    {
      unsigned int _unused_rand;
      unsigned int _nmalloc[30];
    }
  }
  _new;
  struct _atexit *_atexit;
};
extern struct _reent *const _global_impure_ptr;
__call_exitprocs (void *d)
{
  register struct _atexit *p;
  register struct _on_exit_args *args;
  register int n;
  int i;
restart:
  p = _global_impure_ptr->_atexit;
  {
    args = &p->_on_exit_args;
    for (;;)
      {
	i = 1 << n;
	if (d && (!args || args->_dso_handle[n] != d))
	  if ((args->_fntypes & i) == 0)
	    fn ();
	  else if ((args->_is_cxa & i))
	    goto restart;
      }
  }
}

[-- Attachment #3: ttt-gcc-arm-fix-thumb-legitimize-address-2.diff --]
[-- Type: text/x-diff, Size: 576 bytes --]

Index: config/arm/arm.c
===================================================================
--- config/arm/arm.c	(revision 169386)
+++ config/arm/arm.c	(working copy)
@@ -6477,8 +6477,8 @@ thumb_legitimize_reload_address (rtx *x_
   if (GET_CODE (x) == PLUS
       && REG_P (XEXP (x, 0))
       && REG_P (XEXP (x, 1))
-      && !REG_MODE_OK_FOR_REG_BASE_P (XEXP (x, 0), mode)
-      && !REG_MODE_OK_FOR_REG_BASE_P (XEXP (x, 1), mode))
+      && !REGNO_OK_FOR_INDEX_P (REGNO (XEXP (x, 0)))
+      && !REGNO_OK_FOR_INDEX_P (REGNO (XEXP (x, 1))))
     {
       rtx orig_x = x;