| Message ID | 20240816101241.2973252-1-torbjorn.svensson@foss.st.com |
|---|---|
| State | New |
| Headers | show |
| Series | testsuite: Add -fwrapv to signbit-5.c | expand |
On 8/16/24 4:12 AM, Torbjörn SVENSSON wrote: > Ok for trunk and releases/gcc-14? > > Verified this on x86_64 and arm-none-eabi. > Don't know if the other "truth type" dg-lines can be removed as well. > > -- > > On Cortex-M55 with MVE, the test case fails due to -INT_MAX being > undefined. Adding -fwrapv solves the issues. > > Regtested on x86_64-pc-linux and arm-none-eabi for > Cortex-M0/M3/M4/M7/M33/M55/M85/A7. > > gcc/testsuite/ChangeLog: > > * gcc.dg/signbit-5.c: Add -fwrapv and remove x86 exception. Presumably the -x[i] when i == 0 cases? I'm a bit surprised that doing a -INT_MIN didn't produce -INT_MIN, but it's still a bad thing to do due to the overflow. So, OK for the trunk and release branches. If we need to adjust risc-v we'll know if a few days :-) jeff
On 2024-08-16 16:07, Jeff Law wrote: > > > On 8/16/24 4:12 AM, Torbjörn SVENSSON wrote: >> Ok for trunk and releases/gcc-14? >> >> Verified this on x86_64 and arm-none-eabi. >> Don't know if the other "truth type" dg-lines can be removed as well. >> >> -- >> >> On Cortex-M55 with MVE, the test case fails due to -INT_MAX being >> undefined. Adding -fwrapv solves the issues. >> >> Regtested on x86_64-pc-linux and arm-none-eabi for >> Cortex-M0/M3/M4/M7/M33/M55/M85/A7. >> >> gcc/testsuite/ChangeLog: >> >> * gcc.dg/signbit-5.c: Add -fwrapv and remove x86 exception. > Presumably the -x[i] when i == 0 cases? I'm a bit surprised that doing > a -INT_MIN didn't produce -INT_MIN, but it's still a bad thing to do due > to the overflow. On the Cortex-M55 with MVE, -INT_MIN will result in INT_MIN, i.e. a large negative value. The negated INT_MIN value cannot be represented using the two complement form with the same number of bits. > So, OK for the trunk and release branches. If we need to adjust risc-v > we'll know if a few days :-) Ok. Pushed as r15-2950 and r14-10592. Kind regards, Torbjörn
On Fri, Aug 16, 2024 at 4:30 PM Torbjorn SVENSSON <torbjorn.svensson@foss.st.com> wrote: > > > > On 2024-08-16 16:07, Jeff Law wrote: > > > > > > On 8/16/24 4:12 AM, Torbjörn SVENSSON wrote: > >> Ok for trunk and releases/gcc-14? > >> > >> Verified this on x86_64 and arm-none-eabi. > >> Don't know if the other "truth type" dg-lines can be removed as well. > >> > >> -- > >> > >> On Cortex-M55 with MVE, the test case fails due to -INT_MAX being > >> undefined. Adding -fwrapv solves the issues. > >> > >> Regtested on x86_64-pc-linux and arm-none-eabi for > >> Cortex-M0/M3/M4/M7/M33/M55/M85/A7. > >> > >> gcc/testsuite/ChangeLog: > >> > >> * gcc.dg/signbit-5.c: Add -fwrapv and remove x86 exception. > > Presumably the -x[i] when i == 0 cases? I'm a bit surprised that doing > > a -INT_MIN didn't produce -INT_MIN, but it's still a bad thing to do due > > to the overflow. > > On the Cortex-M55 with MVE, -INT_MIN will result in INT_MIN, i.e. a > large negative value. The negated INT_MIN value cannot be represented > using the two complement form with the same number of bits. Note this is what should happen everywhere. I think the testcase should simply avoid this special value - not sure why it was written this way. We shouldn't turn the test into -fwrapv only. Tamar, why did you cover negating INT_MIN here? Can we use INT_MIN + 1 instead? Richard. > > > So, OK for the trunk and release branches. If we need to adjust risc-v > > we'll know if a few days :-) > > Ok. > > > Pushed as r15-2950 and r14-10592. > > Kind regards, > Torbjörn
> -----Original Message----- > From: Richard Biener <richard.guenther@gmail.com> > Sent: Tuesday, August 20, 2024 12:33 PM > To: Torbjorn SVENSSON <torbjorn.svensson@foss.st.com> > Cc: Jeff Law <jeffreyalaw@gmail.com>; gcc-patches@gcc.gnu.org; Richard > Earnshaw <Richard.Earnshaw@arm.com>; quic_apinski@quicinc.com; > yvan.roux@foss.st.com; Tamar Christina <Tamar.Christina@arm.com> > Subject: Re: [PATCH] testsuite: Add -fwrapv to signbit-5.c > > On Fri, Aug 16, 2024 at 4:30 PM Torbjorn SVENSSON > <torbjorn.svensson@foss.st.com> wrote: > > > > > > > > On 2024-08-16 16:07, Jeff Law wrote: > > > > > > > > > On 8/16/24 4:12 AM, Torbjörn SVENSSON wrote: > > >> Ok for trunk and releases/gcc-14? > > >> > > >> Verified this on x86_64 and arm-none-eabi. > > >> Don't know if the other "truth type" dg-lines can be removed as well. > > >> > > >> -- > > >> > > >> On Cortex-M55 with MVE, the test case fails due to -INT_MAX being > > >> undefined. Adding -fwrapv solves the issues. > > >> > > >> Regtested on x86_64-pc-linux and arm-none-eabi for > > >> Cortex-M0/M3/M4/M7/M33/M55/M85/A7. > > >> > > >> gcc/testsuite/ChangeLog: > > >> > > >> * gcc.dg/signbit-5.c: Add -fwrapv and remove x86 exception. > > > Presumably the -x[i] when i == 0 cases? I'm a bit surprised that doing > > > a -INT_MIN didn't produce -INT_MIN, but it's still a bad thing to do due > > > to the overflow. > > > > On the Cortex-M55 with MVE, -INT_MIN will result in INT_MIN, i.e. a > > large negative value. The negated INT_MIN value cannot be represented > > using the two complement form with the same number of bits. > > Note this is what should happen everywhere. I think the testcase should simply > avoid this special value - not sure why it was written this way. We shouldn't > turn the test into -fwrapv only. Tamar, why did you cover negating > INT_MIN here? Precisely because it is the special case. while -INT_MIN is undefined behaviour unless -fwrapv it was hiding the constant from the language to test that the implementation defined behavior from scalar and vector matched. As you pointed out, -INT_MIN should be INT_MIN on most architectures. and that's what the test expects. However right shift of a negative value is Implementation defined. This likely indicates that this isn't the same for vector and scalar on MVE. I guess in hindsight the test is doing too much as it also is testing if the code can be vectorized, and the implementation defined behavior testing belongs in the backend. (this was 4 years ago, live and learn 😊). > Can we use INT_MIN + 1 instead? Yes, think that's the better solution, testing -fwrapv here doesn't seem right as signbit-4.c already tests the wrapv behavior just with scalar. And also using -fwrapv tests a different part of the code as the pattern is guarded by TYPE_OVERFLOW_UNDEFINED. Thanks, Tamar > > Richard. > > > > > > So, OK for the trunk and release branches. If we need to adjust risc-v > > > we'll know if a few days :-) > > > > Ok. > > > > > > Pushed as r15-2950 and r14-10592. > > > > Kind regards, > > Torbjörn
On 2024-08-20 14:37, Tamar Christina wrote: >> -----Original Message----- >> From: Richard Biener <richard.guenther@gmail.com> >> Sent: Tuesday, August 20, 2024 12:33 PM >> To: Torbjorn SVENSSON <torbjorn.svensson@foss.st.com> >> Cc: Jeff Law <jeffreyalaw@gmail.com>; gcc-patches@gcc.gnu.org; Richard >> Earnshaw <Richard.Earnshaw@arm.com>; quic_apinski@quicinc.com; >> yvan.roux@foss.st.com; Tamar Christina <Tamar.Christina@arm.com> >> Subject: Re: [PATCH] testsuite: Add -fwrapv to signbit-5.c >> >> On Fri, Aug 16, 2024 at 4:30 PM Torbjorn SVENSSON >> <torbjorn.svensson@foss.st.com> wrote: >>> >>> >>> >>> On 2024-08-16 16:07, Jeff Law wrote: >>>> >>>> >>>> On 8/16/24 4:12 AM, Torbjörn SVENSSON wrote: >>>>> Ok for trunk and releases/gcc-14? >>>>> >>>>> Verified this on x86_64 and arm-none-eabi. >>>>> Don't know if the other "truth type" dg-lines can be removed as well. >>>>> >>>>> -- >>>>> >>>>> On Cortex-M55 with MVE, the test case fails due to -INT_MAX being >>>>> undefined. Adding -fwrapv solves the issues. >>>>> >>>>> Regtested on x86_64-pc-linux and arm-none-eabi for >>>>> Cortex-M0/M3/M4/M7/M33/M55/M85/A7. >>>>> >>>>> gcc/testsuite/ChangeLog: >>>>> >>>>> * gcc.dg/signbit-5.c: Add -fwrapv and remove x86 exception. >>>> Presumably the -x[i] when i == 0 cases? I'm a bit surprised that doing >>>> a -INT_MIN didn't produce -INT_MIN, but it's still a bad thing to do due >>>> to the overflow. >>> >>> On the Cortex-M55 with MVE, -INT_MIN will result in INT_MIN, i.e. a >>> large negative value. The negated INT_MIN value cannot be represented >>> using the two complement form with the same number of bits. >> >> Note this is what should happen everywhere. I think the testcase should simply >> avoid this special value - not sure why it was written this way. We shouldn't >> turn the test into -fwrapv only. Tamar, why did you cover negating >> INT_MIN here? > > Precisely because it is the special case. while -INT_MIN is undefined behaviour > unless -fwrapv it was hiding the constant from the language to test that the > implementation defined behavior from scalar and vector matched. > > As you pointed out, -INT_MIN should be INT_MIN on most architectures. > and that's what the test expects. However right shift of a negative value is > Implementation defined. This likely indicates that this isn't the same for > vector and scalar on MVE. > > I guess in hindsight the test is doing too much as it also is testing if the code > can be vectorized, and the implementation defined behavior testing belongs > in the backend. (this was 4 years ago, live and learn 😊). > >> Can we use INT_MIN + 1 instead? > > Yes, think that's the better solution, testing -fwrapv here doesn't seem right > as signbit-4.c already tests the wrapv behavior just with scalar. And also using > -fwrapv tests a different part of the code as the pattern is guarded by > TYPE_OVERFLOW_UNDEFINED. Are you going to provide a patch chaining this or am I supposed to do it? I don't mind doing it, but I have a feeling that you know better what is expected or not here than I do. :) Kind regards, Torbjörn > > Thanks, > Tamar >> >> Richard. >> >>> >>>> So, OK for the trunk and release branches. If we need to adjust risc-v >>>> we'll know if a few days :-) >>> >>> Ok. >>> >>> >>> Pushed as r15-2950 and r14-10592. >>> >>> Kind regards, >>> Torbjörn
> -----Original Message----- > From: Torbjorn SVENSSON <torbjorn.svensson@foss.st.com> > Sent: Wednesday, August 21, 2024 2:23 PM > To: Tamar Christina <Tamar.Christina@arm.com>; Richard Biener > <richard.guenther@gmail.com> > Cc: Jeff Law <jeffreyalaw@gmail.com>; gcc-patches@gcc.gnu.org; Richard > Earnshaw <Richard.Earnshaw@arm.com>; quic_apinski@quicinc.com; > yvan.roux@foss.st.com > Subject: Re: [PATCH] testsuite: Add -fwrapv to signbit-5.c > > > > On 2024-08-20 14:37, Tamar Christina wrote: > >> -----Original Message----- > >> From: Richard Biener <richard.guenther@gmail.com> > >> Sent: Tuesday, August 20, 2024 12:33 PM > >> To: Torbjorn SVENSSON <torbjorn.svensson@foss.st.com> > >> Cc: Jeff Law <jeffreyalaw@gmail.com>; gcc-patches@gcc.gnu.org; Richard > >> Earnshaw <Richard.Earnshaw@arm.com>; quic_apinski@quicinc.com; > >> yvan.roux@foss.st.com; Tamar Christina <Tamar.Christina@arm.com> > >> Subject: Re: [PATCH] testsuite: Add -fwrapv to signbit-5.c > >> > >> On Fri, Aug 16, 2024 at 4:30 PM Torbjorn SVENSSON > >> <torbjorn.svensson@foss.st.com> wrote: > >>> > >>> > >>> > >>> On 2024-08-16 16:07, Jeff Law wrote: > >>>> > >>>> > >>>> On 8/16/24 4:12 AM, Torbjörn SVENSSON wrote: > >>>>> Ok for trunk and releases/gcc-14? > >>>>> > >>>>> Verified this on x86_64 and arm-none-eabi. > >>>>> Don't know if the other "truth type" dg-lines can be removed as well. > >>>>> > >>>>> -- > >>>>> > >>>>> On Cortex-M55 with MVE, the test case fails due to -INT_MAX being > >>>>> undefined. Adding -fwrapv solves the issues. > >>>>> > >>>>> Regtested on x86_64-pc-linux and arm-none-eabi for > >>>>> Cortex-M0/M3/M4/M7/M33/M55/M85/A7. > >>>>> > >>>>> gcc/testsuite/ChangeLog: > >>>>> > >>>>> * gcc.dg/signbit-5.c: Add -fwrapv and remove x86 exception. > >>>> Presumably the -x[i] when i == 0 cases? I'm a bit surprised that doing > >>>> a -INT_MIN didn't produce -INT_MIN, but it's still a bad thing to do due > >>>> to the overflow. > >>> > >>> On the Cortex-M55 with MVE, -INT_MIN will result in INT_MIN, i.e. a > >>> large negative value. The negated INT_MIN value cannot be represented > >>> using the two complement form with the same number of bits. > >> > >> Note this is what should happen everywhere. I think the testcase should simply > >> avoid this special value - not sure why it was written this way. We shouldn't > >> turn the test into -fwrapv only. Tamar, why did you cover negating > >> INT_MIN here? > > > > Precisely because it is the special case. while -INT_MIN is undefined behaviour > > unless -fwrapv it was hiding the constant from the language to test that the > > implementation defined behavior from scalar and vector matched. > > > > As you pointed out, -INT_MIN should be INT_MIN on most architectures. > > and that's what the test expects. However right shift of a negative value is > > Implementation defined. This likely indicates that this isn't the same for > > vector and scalar on MVE. > > > > I guess in hindsight the test is doing too much as it also is testing if the code > > can be vectorized, and the implementation defined behavior testing belongs > > in the backend. (this was 4 years ago, live and learn 😊). > > > >> Can we use INT_MIN + 1 instead? > > > > Yes, think that's the better solution, testing -fwrapv here doesn't seem right > > as signbit-4.c already tests the wrapv behavior just with scalar. And also using > > -fwrapv tests a different part of the code as the pattern is guarded by > > TYPE_OVERFLOW_UNDEFINED. > > Are you going to provide a patch chaining this or am I supposed to do it? > I don't mind doing it, but I have a feeling that you know better what is > expected or not here than I do. :) Sure, I'll submit one when I'm back next week. Tamar > > Kind regards, > Torbjörn > > > > > Thanks, > > Tamar > >> > >> Richard. > >> > >>> > >>>> So, OK for the trunk and release branches. If we need to adjust risc-v > >>>> we'll know if a few days :-) > >>> > >>> Ok. > >>> > >>> > >>> Pushed as r15-2950 and r14-10592. > >>> > >>> Kind regards, > >>> Torbjörn
diff --git a/gcc/testsuite/gcc.dg/signbit-5.c b/gcc/testsuite/gcc.dg/signbit-5.c index 1e1b237a0e0..2bca640f930 100644 --- a/gcc/testsuite/gcc.dg/signbit-5.c +++ b/gcc/testsuite/gcc.dg/signbit-5.c @@ -1,8 +1,7 @@ /* { dg-do run } */ -/* { dg-options "-O3" } */ +/* { dg-options "-O3 -fwrapv" } */ /* This test does not work when the truth type does not match vector type. */ -/* { dg-additional-options "-mno-avx512f" { target { i?86-*-* x86_64-*-* } } } */ /* { dg-additional-options "-march=armv8-a" { target aarch64_sve } } */ /* { dg-xfail-run-if "truth type does not match vector type" { amdgcn-*-* } } */ /* { dg-xfail-run-if "truth type does not match vector type" { riscv_v } } */
Ok for trunk and releases/gcc-14? Verified this on x86_64 and arm-none-eabi. Don't know if the other "truth type" dg-lines can be removed as well. -- On Cortex-M55 with MVE, the test case fails due to -INT_MAX being undefined. Adding -fwrapv solves the issues. Regtested on x86_64-pc-linux and arm-none-eabi for Cortex-M0/M3/M4/M7/M33/M55/M85/A7. gcc/testsuite/ChangeLog: * gcc.dg/signbit-5.c: Add -fwrapv and remove x86 exception. Signed-off-by: Torbjörn SVENSSON <torbjorn.svensson@foss.st.com> Co-authored-by: Yvan ROUX <yvan.roux@foss.st.com> --- gcc/testsuite/gcc.dg/signbit-5.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)