RE: [PATCH, ARM, iWMMXT] Check IWMMXT_GR_REGS in the SECONDARY_RELOAD MACRO

[Xinyu Qi <xyqi at marvell dot com>]

At 2013-05-24 15:19:36,"Chung-Ju Wu" <jasonwucj@gmail.com> wrote: 
> 2013/5/24 Xinyu Qi <xyqi@marvell.com>:
> > Hi,
> >
> >   For this simple case, compiled with option -march=iwmmxt -O, #define
> > N 64 signed int b[N]; signed long long j[N], d[N]; void foo (void) {
> >   int i;
> >   for (i = 0; i < N; i++)
> >     j[i] = d[i] << b[i];
> > }
> > An internal compiler error occurs,
> > error: insn does not satisfy its constraints:
> > (insn 112 74 75 3 (set (reg:SI 96 wcgr0)
> >         (mem/c:SI (plus:SI (reg:SI 3 r3 [orig:174 ivtmp.19 ] [174])
> >                 (reg/f:SI 0 r0 [183])) [0 MEM[symbol: b, index:
> ivtmp.19_14, offset: 0B]+0 S4 A32])) {*iwmmxt_movsi_insn}
> >      (nil))
> >
> > The load address of wmmx wcgr register cannot accept the register offset
> mode and the reload pass fails to fix it, so that such error happens.
> > This issue could be solved by adding check code for IWMMXT_GR_REGS class
> in the SECONDARY_RELOAD MACRO if TARGET_IWMMXT. Current code only
> check the IWMMXT_REGS group.
> > Patch attached with a new test.
> > Pass full dejagnu test. No regression.
> >
> > Is this fix proper?
> > OK for trunk?
> >
> 
> I cannot approve it.  But here are some comments and hope it helps.

Hi Chung-Ju,

Thanks for your comments:)
I fixed the typo you mentioned and regenerated the patch attached.

ChangeLog
gcc/
2013-05-24  Xinyu Qi  <xyqi@marvell.com>

        * config/arm/arm.h (SECONDARY_OUTPUT_RELOAD_CLASS): Check
        IWMMXT_GR_REGS register class.
        (SECONDARY_INPUT_RELOAD_CLASS): Likewise.

testsuite/
2013-05-24  Xinyu Qi  <xyqi@marvell.com>

	* gcc.target/arm/mmx-3.c: New test.

Thanks,
Xinyu

> 
> 
> > ChangeLog
> > gcc/
> > 2013-05-24  Xinyu Qi  <xyqi@marvell.com>
> >
> >         * config/arm/arm.h (SECONDARY_OUTPUT_RELOAD_CLASS):
> Check IWMMXT_GR_REGS.
> 
> This line just ends at 81 column.
> How about this one?
> 
> 2013-05-24  Xinyu Qi  <xyqi@marvell.com>
> 
>         * config/arm/arm.h (SECONDARY_OUTPUT_RELOAD_CLASS): Check
>         IWMMXT_GR_REGS register class.
>         (SECONDARY_INPUT_RELOAD_CLASS): Likewise.
> 
> >
> > testsuite/
> > 2013-05-24  Xinyu Qi  <xyqi@marvell.com>
> >
> >         * gcc.target/arm/mmx-3.c: New test.
> 
> 
> > Index: gcc/config/arm/arm.h
> >
> ================================================================
> ===
> > --- gcc/config/arm/arm.h    (revision 199090)
> > +++ gcc/config/arm/arm.h    (working copy)
> > @@ -1280,7 +1280,8 @@
> >    ((TARGET_VFP && TARGET_HARD_FLOAT                \
> >      && IS_VFP_CLASS (CLASS))                    \
> >     ? coproc_secondary_reload_class (MODE, X, FALSE)        \
> > -   : (TARGET_IWMMXT && (CLASS) == IWMMXT_REGS)            \
> > +   : (TARGET_IWMMXT && ((CLASS) == IWMMXT_REGS)            \
> > +                        || (CLASS) == IWMMXT_GR_REGS)        \
> 
> I think it should be
> 
> +   : (TARGET_IWMMXT && ((CLASS) == IWMMXT_REGS            \
> +                        || (CLASS) == IWMMXT_GR_REGS))        \
> 
> 
> >     ? coproc_secondary_reload_class (MODE, X, TRUE)        \
> >     : TARGET_32BIT                        \
> >     ? (((MODE) == HImode && ! arm_arch4 && true_regnum (X) == -1) \
> @@
> > -1293,7 +1294,8 @@
> >    ((TARGET_VFP && TARGET_HARD_FLOAT                \
> >      && IS_VFP_CLASS (CLASS))                    \
> >      ? coproc_secondary_reload_class (MODE, X, FALSE) :        \
> > -    (TARGET_IWMMXT && (CLASS) == IWMMXT_REGS) ?            \
> > +    (TARGET_IWMMXT && ((CLASS) == IWMMXT_REGS            \
> > +                       || (CLASS) == IWMMXT_GR_REGS)) ?        \
> >      coproc_secondary_reload_class (MODE, X, TRUE) :        \
> >     (TARGET_32BIT ?                        \
> >      (((CLASS) == IWMMXT_REGS || (CLASS) == IWMMXT_GR_REGS)    \
> 
> It seems that you didn't CC arm maintainer.
> Let me do this for you. :)
> 
> 
> Best regards,
> jasonwucj

Attachment: GR_secondary_reload.diff
Description: GR_secondary_reload.diff