* [ARM] [1/2] Make ARM_LEGITIMIZE_RELOAD_ADDRESS be a function
@ 2010-12-22 11:46 Jie Zhang
2011-01-17 2:34 ` PING " Jie Zhang
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Jie Zhang @ 2010-12-22 11:46 UTC (permalink / raw)
To: gcc-patches
[-- 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)
^ permalink raw reply [flat|nested] 10+ messages in thread
* PING [ARM] [1/2] Make ARM_LEGITIMIZE_RELOAD_ADDRESS be a function
2010-12-22 11:46 [ARM] [1/2] Make ARM_LEGITIMIZE_RELOAD_ADDRESS be a function Jie Zhang
@ 2011-01-17 2:34 ` Jie Zhang
2011-01-27 2:24 ` Ramana Radhakrishnan
2011-01-28 15:29 ` Richard Earnshaw
2 siblings, 0 replies; 10+ messages in thread
From: Jie Zhang @ 2011-01-17 2:34 UTC (permalink / raw)
To: gcc-patches, Richard Earnshaw
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [ARM] [1/2] Make ARM_LEGITIMIZE_RELOAD_ADDRESS be a function
2010-12-22 11:46 [ARM] [1/2] Make ARM_LEGITIMIZE_RELOAD_ADDRESS be a function Jie Zhang
2011-01-17 2:34 ` PING " Jie Zhang
@ 2011-01-27 2:24 ` Ramana Radhakrishnan
2011-01-27 3:29 ` Jie Zhang
2011-01-28 15:29 ` Richard Earnshaw
2 siblings, 1 reply; 10+ messages in thread
From: Ramana Radhakrishnan @ 2011-01-27 2:24 UTC (permalink / raw)
To: Jie Zhang; +Cc: gcc-patches, Richard Earnshaw
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [ARM] [1/2] Make ARM_LEGITIMIZE_RELOAD_ADDRESS be a function
2011-01-27 2:24 ` Ramana Radhakrishnan
@ 2011-01-27 3:29 ` Jie Zhang
0 siblings, 0 replies; 10+ messages in thread
From: Jie Zhang @ 2011-01-27 3:29 UTC (permalink / raw)
To: Ramana Radhakrishnan; +Cc: gcc-patches, Richard Earnshaw
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [ARM] [1/2] Make ARM_LEGITIMIZE_RELOAD_ADDRESS be a function
2010-12-22 11:46 [ARM] [1/2] Make ARM_LEGITIMIZE_RELOAD_ADDRESS be a function Jie Zhang
2011-01-17 2:34 ` PING " Jie Zhang
2011-01-27 2:24 ` Ramana Radhakrishnan
@ 2011-01-28 15:29 ` Richard Earnshaw
2011-01-28 18:00 ` Jie Zhang
2 siblings, 1 reply; 10+ messages in thread
From: Richard Earnshaw @ 2011-01-28 15:29 UTC (permalink / raw)
To: Jie Zhang; +Cc: gcc-patches
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [ARM] [1/2] Make ARM_LEGITIMIZE_RELOAD_ADDRESS be a function
2011-01-28 15:29 ` Richard Earnshaw
@ 2011-01-28 18:00 ` Jie Zhang
2011-01-28 18:07 ` Richard Earnshaw
0 siblings, 1 reply; 10+ messages in thread
From: Jie Zhang @ 2011-01-28 18:00 UTC (permalink / raw)
To: Richard Earnshaw; +Cc: gcc-patches
[-- 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);
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [ARM] [1/2] Make ARM_LEGITIMIZE_RELOAD_ADDRESS be a function
2011-01-28 18:00 ` Jie Zhang
@ 2011-01-28 18:07 ` Richard Earnshaw
2011-01-29 10:14 ` Jie Zhang
0 siblings, 1 reply; 10+ messages in thread
From: Richard Earnshaw @ 2011-01-28 18:07 UTC (permalink / raw)
To: Jie Zhang; +Cc: gcc-patches
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [ARM] [1/2] Make ARM_LEGITIMIZE_RELOAD_ADDRESS be a function
2011-01-28 18:07 ` Richard Earnshaw
@ 2011-01-29 10:14 ` Jie Zhang
2011-01-30 12:34 ` Jie Zhang
0 siblings, 1 reply; 10+ messages in thread
From: Jie Zhang @ 2011-01-29 10:14 UTC (permalink / raw)
To: Richard Earnshaw; +Cc: gcc-patches
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [ARM] [1/2] Make ARM_LEGITIMIZE_RELOAD_ADDRESS be a function
2011-01-29 10:14 ` Jie Zhang
@ 2011-01-30 12:34 ` Jie Zhang
2011-01-30 12:40 ` Jie Zhang
0 siblings, 1 reply; 10+ messages in thread
From: Jie Zhang @ 2011-01-30 12:34 UTC (permalink / raw)
To: Richard Earnshaw; +Cc: gcc-patches
[-- 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;
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [ARM] [1/2] Make ARM_LEGITIMIZE_RELOAD_ADDRESS be a function
2011-01-30 12:34 ` Jie Zhang
@ 2011-01-30 12:40 ` Jie Zhang
0 siblings, 0 replies; 10+ messages in thread
From: Jie Zhang @ 2011-01-30 12:40 UTC (permalink / raw)
To: Richard Earnshaw; +Cc: gcc-patches
[-- 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;
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2011-01-30 8:40 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-12-22 11:46 [ARM] [1/2] Make ARM_LEGITIMIZE_RELOAD_ADDRESS be a function Jie Zhang
2011-01-17 2:34 ` PING " Jie Zhang
2011-01-27 2:24 ` Ramana Radhakrishnan
2011-01-27 3:29 ` Jie Zhang
2011-01-28 15:29 ` Richard Earnshaw
2011-01-28 18:00 ` Jie Zhang
2011-01-28 18:07 ` Richard Earnshaw
2011-01-29 10:14 ` Jie Zhang
2011-01-30 12:34 ` Jie Zhang
2011-01-30 12:40 ` Jie Zhang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).