Message ID | CACgzC7Ah7+rBUXCej2dHD3y-oVtpgrRKMA9y+e=h7jEk=ZmLOQ@mail.gmail.com |
---|---|
State | New |
Headers | show |
Not yet. Why is this not a problem in the LT / UNLT case ? The testcase is not correctly written. @@ -0,0 +1,16 @@ +/* { dg-do compile } */ +/* { dg-options "-O3 -mfpu=neon -mcpu=cortex-a9 -mthumb -mfloat-abi=hard -S" } */ dg-add-options arm_neon ? dg-require-effective-target arm_neon ? regards Ramana On Wed, Jun 26, 2013 at 9:01 AM, Zhenqiang Chen <zhenqiang.chen@linaro.org> wrote: > On 18 June 2013 17:41, Ramana Radhakrishnan <ramrad01@arm.com> wrote: >> On 06/18/13 09:50, Zhenqiang Chen wrote: >>> >>> Hi, >>> >>> During expand, function vcond<mode><mode> inverses some CMP, e.g. >>> >>> a LE b -> b GE a >>> >>> But if "b" is "CONST0_RTX", "b GE a" will be an illegal insn. >>> >>> (insn 933 932 934 113 (set (reg:V4SI 1027) >>> (unspec:V4SI [ >>> (const_vector:V4SI [ >>> (const_int 0 [0]) >>> (const_int 0 [0]) >>> (const_int 0 [0]) >>> (const_int 0 [0]) >>> ]) >>> (reg:V4SI 1023 [ vect_var_.49 ]) >>> (const_int 1 [0x1]) >>> ] UNSPEC_VCGE)) PUGHSlab/Mapping.c:567 -1 >>> (nil)) >>> >>> Refer https://bugs.launchpad.net/linaro-toolchain-binaries/+bug/1189445 >>> for more. And the bug also happens for FSF trunk. >>> >>> The similar issue >>> (https://bugs.launchpad.net/linaro-toolchain-binaries/+bug/1163942) >>> had fixed on AARCH64: >>> http://gcc.gnu.org/ml/gcc-patches/2013-04/msg00581.html >>> >>> The patch is similar to the fix for aarch64. >>> >>> Bootstrap and no make check regression on Panda Board. >>> >>> Is it OK for trunk and 4.8? >> >> >> No, not without an appropriate set of testcases that exercise these cases. > > Thanks for the comments. Patch is updated with a test case. > > diff --git a/gcc/config/arm/neon.md b/gcc/config/arm/neon.md > index 2761adb..6d9f604 100644 > --- a/gcc/config/arm/neon.md > +++ b/gcc/config/arm/neon.md > @@ -1710,6 +1710,9 @@ > case LE: > case UNLE: > inverse = 1; > + /* Can not inverse "a LE 0" to "0 GE a". */ > + if (operands[5] == CONST0_RTX (<MODE>mode)) > + inverse = 0; > /* Fall through. */ > case GT: > case UNGT: > diff --git a/gcc/testsuite/gcc.target/arm/lp1189445.c > b/gcc/testsuite/gcc.target/arm/lp1189445.c > new file mode 100644 > index 0000000..8ce4b97 > --- /dev/null > +++ b/gcc/testsuite/gcc.target/arm/lp1189445.c > @@ -0,0 +1,16 @@ > +/* { dg-do compile } */ > +/* { dg-options "-O3 -mfpu=neon -mcpu=cortex-a9 -mthumb > -mfloat-abi=hard -S" } */ > + > +int id; > +int > +test (const long int *data) > +{ > + int i, retval; > + retval = id; > + for (i = 0; i < id; i++) > + { > + retval &= (data[i] <= 0); > + } > + > + return (retval); > +}
On 8 July 2013 20:57, Ramana Radhakrishnan <ramana.gcc@googlemail.com> wrote: > Not yet. Why is this not a problem in the LT / UNLT case ? From the context, after the first switch, only GE/LE/EQ might have operands[5] which is not REG (CONST0_RTX). For others including LT/UNLT, operands[5] should be REG. if (!REG_P (operands[5])) operands[5] = force_reg (<MODE>mode, operands[5]); For GE/LE/EQ, we only reverse LE. So only LE has issue. > The testcase is not correctly written. > > > @@ -0,0 +1,16 @@ > +/* { dg-do compile } */ > +/* { dg-options "-O3 -mfpu=neon -mcpu=cortex-a9 -mthumb > -mfloat-abi=hard -S" } */ > > dg-add-options arm_neon ? > dg-require-effective-target arm_neon ? I will update it. Thanks! -Zhenqiang > On Wed, Jun 26, 2013 at 9:01 AM, Zhenqiang Chen > <zhenqiang.chen@linaro.org> wrote: >> On 18 June 2013 17:41, Ramana Radhakrishnan <ramrad01@arm.com> wrote: >>> On 06/18/13 09:50, Zhenqiang Chen wrote: >>>> >>>> Hi, >>>> >>>> During expand, function vcond<mode><mode> inverses some CMP, e.g. >>>> >>>> a LE b -> b GE a >>>> >>>> But if "b" is "CONST0_RTX", "b GE a" will be an illegal insn. >>>> >>>> (insn 933 932 934 113 (set (reg:V4SI 1027) >>>> (unspec:V4SI [ >>>> (const_vector:V4SI [ >>>> (const_int 0 [0]) >>>> (const_int 0 [0]) >>>> (const_int 0 [0]) >>>> (const_int 0 [0]) >>>> ]) >>>> (reg:V4SI 1023 [ vect_var_.49 ]) >>>> (const_int 1 [0x1]) >>>> ] UNSPEC_VCGE)) PUGHSlab/Mapping.c:567 -1 >>>> (nil)) >>>> >>>> Refer https://bugs.launchpad.net/linaro-toolchain-binaries/+bug/1189445 >>>> for more. And the bug also happens for FSF trunk. >>>> >>>> The similar issue >>>> (https://bugs.launchpad.net/linaro-toolchain-binaries/+bug/1163942) >>>> had fixed on AARCH64: >>>> http://gcc.gnu.org/ml/gcc-patches/2013-04/msg00581.html >>>> >>>> The patch is similar to the fix for aarch64. >>>> >>>> Bootstrap and no make check regression on Panda Board. >>>> >>>> Is it OK for trunk and 4.8? >>> >>> >>> No, not without an appropriate set of testcases that exercise these cases. >> >> Thanks for the comments. Patch is updated with a test case. >> >> diff --git a/gcc/config/arm/neon.md b/gcc/config/arm/neon.md >> index 2761adb..6d9f604 100644 >> --- a/gcc/config/arm/neon.md >> +++ b/gcc/config/arm/neon.md >> @@ -1710,6 +1710,9 @@ >> case LE: >> case UNLE: >> inverse = 1; >> + /* Can not inverse "a LE 0" to "0 GE a". */ >> + if (operands[5] == CONST0_RTX (<MODE>mode)) >> + inverse = 0; >> /* Fall through. */ >> case GT: >> case UNGT: >> diff --git a/gcc/testsuite/gcc.target/arm/lp1189445.c >> b/gcc/testsuite/gcc.target/arm/lp1189445.c >> new file mode 100644 >> index 0000000..8ce4b97 >> --- /dev/null >> +++ b/gcc/testsuite/gcc.target/arm/lp1189445.c >> @@ -0,0 +1,16 @@ >> +/* { dg-do compile } */ >> +/* { dg-options "-O3 -mfpu=neon -mcpu=cortex-a9 -mthumb >> -mfloat-abi=hard -S" } */ >> + >> +int id; >> +int >> +test (const long int *data) >> +{ >> + int i, retval; >> + retval = id; >> + for (i = 0; i < id; i++) >> + { >> + retval &= (data[i] <= 0); >> + } >> + >> + return (retval); >> +}
On Mon, Jul 08, 2013 at 04:32:13PM +0100, Zhenqiang Chen wrote: > On 8 July 2013 20:57, Ramana Radhakrishnan <ramana.gcc@googlemail.com> wrote: > > Not yet. Why is this not a problem in the LT / UNLT case ? > > From the context, after the first switch, only GE/LE/EQ might have > operands[5] which is not REG (CONST0_RTX). For others including > LT/UNLT, operands[5] should be REG. This is true, but looks like an omission. My copy of the ARMARM has immediate #0 instruction forms for CMLT, CMLE, CMGE, CMGT and CMEQ. Perhaps it is beyond the scope of your bugfix (though it was in your original patch?), but this should be fixed in future so as not to force 0 to registers. > > if (!REG_P (operands[5])) > operands[5] = force_reg (<MODE>mode, operands[5]); > > For GE/LE/EQ, we only reverse LE. So only LE has issue. > For now, but as above - as soon as this code is fixed to generate immediate #0 forms, it will be fragile again. > >> diff --git a/gcc/config/arm/neon.md b/gcc/config/arm/neon.md > >> index 2761adb..6d9f604 100644 > >> --- a/gcc/config/arm/neon.md > >> +++ b/gcc/config/arm/neon.md > >> @@ -1710,6 +1710,9 @@ > >> case LE: > >> case UNLE: > >> inverse = 1; > >> + /* Can not inverse "a LE 0" to "0 GE a". */ > >> + if (operands[5] == CONST0_RTX (<MODE>mode)) > >> + inverse = 0; > >> /* Fall through. */ > >> case GT: > >> case UNGT: Is this really what you mean? Surely now you will have: inverse = 0 base_comparison = gen_neon_vcgt Thus in the next switch you will call: emit_insn (gen_neon_vcgt (mask, operands[4], operands[5], magic_rtx)); Which looks wrong. Would you not also have to set swap_bsl_operands to get back to the correct semantics? > >> diff --git a/gcc/testsuite/gcc.target/arm/lp1189445.c > >> b/gcc/testsuite/gcc.target/arm/lp1189445.c > >> new file mode 100644 > >> index 0000000..8ce4b97 > >> --- /dev/null > >> +++ b/gcc/testsuite/gcc.target/arm/lp1189445.c > >> @@ -0,0 +1,16 @@ > >> +/* { dg-do compile } */ > >> +/* { dg-options "-O3 -mfpu=neon -mcpu=cortex-a9 -mthumb > >> -mfloat-abi=hard -S" } */ > >> + > >> +int id; > >> +int > >> +test (const long int *data) > >> +{ > >> + int i, retval; > >> + retval = id; > >> + for (i = 0; i < id; i++) > >> + { > >> + retval &= (data[i] <= 0); > >> + } > >> + > >> + return (retval); > >> +} > This testcase is not much use. It may well compile, but won't catch the wrong instruction generation issue I pointed out above. I much prefer your original patch, with a more rigorous testcase. Thanks, James
On 07/08/2013 08:32 AM, Zhenqiang Chen wrote: > On 8 July 2013 20:57, Ramana Radhakrishnan <ramana.gcc@googlemail.com> wrote: >> Not yet. Why is this not a problem in the LT / UNLT case ? > >>From the context, after the first switch, only GE/LE/EQ might have > operands[5] which is not REG (CONST0_RTX). For others including > LT/UNLT, operands[5] should be REG. > > if (!REG_P (operands[5])) > operands[5] = force_reg (<MODE>mode, operands[5]); > > For GE/LE/EQ, we only reverse LE. So only LE has issue. > >> The testcase is not correctly written. >> >> >> @@ -0,0 +1,16 @@ >> +/* { dg-do compile } */ >> +/* { dg-options "-O3 -mfpu=neon -mcpu=cortex-a9 -mthumb >> -mfloat-abi=hard -S" } */ >> >> dg-add-options arm_neon ? >> dg-require-effective-target arm_neon ? > > I will update it. Please skip the test for multilibs whose flags include -mfpu or -mcpu options, which would conflict with or override the options in the test. Janis > Thanks! > -Zhenqiang > >> On Wed, Jun 26, 2013 at 9:01 AM, Zhenqiang Chen >> <zhenqiang.chen@linaro.org> wrote: >>> On 18 June 2013 17:41, Ramana Radhakrishnan <ramrad01@arm.com> wrote: >>>> On 06/18/13 09:50, Zhenqiang Chen wrote: >>>>> >>>>> Hi, >>>>> >>>>> During expand, function vcond<mode><mode> inverses some CMP, e.g. >>>>> >>>>> a LE b -> b GE a >>>>> >>>>> But if "b" is "CONST0_RTX", "b GE a" will be an illegal insn. >>>>> >>>>> (insn 933 932 934 113 (set (reg:V4SI 1027) >>>>> (unspec:V4SI [ >>>>> (const_vector:V4SI [ >>>>> (const_int 0 [0]) >>>>> (const_int 0 [0]) >>>>> (const_int 0 [0]) >>>>> (const_int 0 [0]) >>>>> ]) >>>>> (reg:V4SI 1023 [ vect_var_.49 ]) >>>>> (const_int 1 [0x1]) >>>>> ] UNSPEC_VCGE)) PUGHSlab/Mapping.c:567 -1 >>>>> (nil)) >>>>> >>>>> Refer https://bugs.launchpad.net/linaro-toolchain-binaries/+bug/1189445 >>>>> for more. And the bug also happens for FSF trunk. >>>>> >>>>> The similar issue >>>>> (https://bugs.launchpad.net/linaro-toolchain-binaries/+bug/1163942) >>>>> had fixed on AARCH64: >>>>> http://gcc.gnu.org/ml/gcc-patches/2013-04/msg00581.html >>>>> >>>>> The patch is similar to the fix for aarch64. >>>>> >>>>> Bootstrap and no make check regression on Panda Board. >>>>> >>>>> Is it OK for trunk and 4.8? >>>> >>>> >>>> No, not without an appropriate set of testcases that exercise these cases. >>> >>> Thanks for the comments. Patch is updated with a test case. >>> >>> diff --git a/gcc/config/arm/neon.md b/gcc/config/arm/neon.md >>> index 2761adb..6d9f604 100644 >>> --- a/gcc/config/arm/neon.md >>> +++ b/gcc/config/arm/neon.md >>> @@ -1710,6 +1710,9 @@ >>> case LE: >>> case UNLE: >>> inverse = 1; >>> + /* Can not inverse "a LE 0" to "0 GE a". */ >>> + if (operands[5] == CONST0_RTX (<MODE>mode)) >>> + inverse = 0; >>> /* Fall through. */ >>> case GT: >>> case UNGT: >>> diff --git a/gcc/testsuite/gcc.target/arm/lp1189445.c >>> b/gcc/testsuite/gcc.target/arm/lp1189445.c >>> new file mode 100644 >>> index 0000000..8ce4b97 >>> --- /dev/null >>> +++ b/gcc/testsuite/gcc.target/arm/lp1189445.c >>> @@ -0,0 +1,16 @@ >>> +/* { dg-do compile } */ >>> +/* { dg-options "-O3 -mfpu=neon -mcpu=cortex-a9 -mthumb >>> -mfloat-abi=hard -S" } */ >>> + >>> +int id; >>> +int >>> +test (const long int *data) >>> +{ >>> + int i, retval; >>> + retval = id; >>> + for (i = 0; i < id; i++) >>> + { >>> + retval &= (data[i] <= 0); >>> + } >>> + >>> + return (retval); >>> +} > >
On Mon, Jul 08, 2013 at 11:44:04AM -0700, Janis Johnson wrote: > >> @@ -0,0 +1,16 @@ > >> +/* { dg-do compile } */ > >> +/* { dg-options "-O3 -mfpu=neon -mcpu=cortex-a9 -mthumb > >> -mfloat-abi=hard -S" } */ > >> > >> dg-add-options arm_neon ? > >> dg-require-effective-target arm_neon ? > > > > I will update it. > > Please skip the test for multilibs whose flags include -mfpu or -mcpu options, > which would conflict with or override the options in the test. Also the -S in dg-options looks wrong. That should be derived from dg-do. Jakub
Thank you all for the comments. The patch is updated as: 1) Revert it to the original one. 2) For the testcase, replace the dg-options with /* { dg-do compile } */ /* { dg-require-effective-target arm_neon } */ /* { dg-add-options arm_neon } */ /* { dg-options "-O3" } */ Bootstrap on Chromebook and Pandaboard. No make check regression on Pandaboard. Thanks! -Zhenqiang On 9 July 2013 02:49, Jakub Jelinek <jakub@redhat.com> wrote: > On Mon, Jul 08, 2013 at 11:44:04AM -0700, Janis Johnson wrote: >> >> @@ -0,0 +1,16 @@ >> >> +/* { dg-do compile } */ >> >> +/* { dg-options "-O3 -mfpu=neon -mcpu=cortex-a9 -mthumb >> >> -mfloat-abi=hard -S" } */ >> >> >> >> dg-add-options arm_neon ? >> >> dg-require-effective-target arm_neon ? >> > >> > I will update it. >> >> Please skip the test for multilibs whose flags include -mfpu or -mcpu options, >> which would conflict with or override the options in the test. > > Also the -S in dg-options looks wrong. That should be derived from dg-do. > > Jakub
Ping? Is it OK for 4.8 and trunk? Thanks! -Zhenqiang On 1 August 2013 10:04, Zhenqiang Chen <zhenqiang.chen@linaro.org> wrote: > Thank you all for the comments. The patch is updated as: > 1) Revert it to the original one. > 2) For the testcase, replace the dg-options with > /* { dg-do compile } */ > /* { dg-require-effective-target arm_neon } */ > /* { dg-add-options arm_neon } */ > /* { dg-options "-O3" } */ > > Bootstrap on Chromebook and Pandaboard. > No make check regression on Pandaboard. > > Thanks! > -Zhenqiang > > On 9 July 2013 02:49, Jakub Jelinek <jakub@redhat.com> wrote: >> On Mon, Jul 08, 2013 at 11:44:04AM -0700, Janis Johnson wrote: >>> >> @@ -0,0 +1,16 @@ >>> >> +/* { dg-do compile } */ >>> >> +/* { dg-options "-O3 -mfpu=neon -mcpu=cortex-a9 -mthumb >>> >> -mfloat-abi=hard -S" } */ >>> >> >>> >> dg-add-options arm_neon ? >>> >> dg-require-effective-target arm_neon ? >>> > >>> > I will update it. >>> >>> Please skip the test for multilibs whose flags include -mfpu or -mcpu options, >>> which would conflict with or override the options in the test. >> >> Also the -S in dg-options looks wrong. That should be derived from dg-do. >> >> Jakub
On 08/01/13 03:04, Zhenqiang Chen wrote: > Thank you all for the comments. The patch is updated as: > 1) Revert it to the original one. > 2) For the testcase, replace the dg-options with > /* { dg-do compile } */ > /* { dg-require-effective-target arm_neon } */ > /* { dg-add-options arm_neon } */ > /* { dg-options "-O3" } */ > > Bootstrap on Chromebook and Pandaboard. > No make check regression on Pandaboard. > > Thanks! > -Zhenqiang Ok for trunk and 4.8 branch. Sorry about the delay. Thanks, Ramana
diff --git a/gcc/config/arm/neon.md b/gcc/config/arm/neon.md index 2761adb..6d9f604 100644 --- a/gcc/config/arm/neon.md +++ b/gcc/config/arm/neon.md @@ -1710,6 +1710,9 @@ case LE: case UNLE: inverse = 1; + /* Can not inverse "a LE 0" to "0 GE a". */ + if (operands[5] == CONST0_RTX (<MODE>mode)) + inverse = 0; /* Fall through. */ case GT: case UNGT: diff --git a/gcc/testsuite/gcc.target/arm/lp1189445.c b/gcc/testsuite/gcc.target/arm/lp1189445.c new file mode 100644 index 0000000..8ce4b97 --- /dev/null +++ b/gcc/testsuite/gcc.target/arm/lp1189445.c @@ -0,0 +1,16 @@ +/* { dg-do compile } */ +/* { dg-options "-O3 -mfpu=neon -mcpu=cortex-a9 -mthumb -mfloat-abi=hard -S" } */ + +int id; +int +test (const long int *data) +{ + int i, retval; + retval = id; + for (i = 0; i < id; i++) + { + retval &= (data[i] <= 0); + } + + return (retval); +}