| Message ID | 4D42F2FC.6080306@codesourcery.com |
|---|---|
| State | New |
| Headers | show |
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.
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,
* 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);