| Message ID | 561E4941.6030204@arm.com |
|---|---|
| State | Accepted |
| Commit | 85f5231d73ab398af64c03e4121ab16c6c172cf6 |
| Headers | show |
Ping. https://gcc.gnu.org/ml/gcc-patches/2015-10/msg01350.html and the backport for 4.9/5 https://gcc.gnu.org/ml/gcc-patches/2015-10/msg01351.html Thanks, Kyrill On 14/10/15 13:23, Kyrill Tkachov wrote: > Hi all, > > This patch fixes the referenced PR by rewriting the vfp3_const_double_for_bits function in arm.c > The function is supposed to accept positive CONST_DOUBLE rtxes whose value is an exact power of 2 > and whose log2 is between 1 and 32. That is values like 2.0, 4.0, 8.9, 16.0 etc... > > The current implementation seems to have been written under the assumption that exact_real_truncate returns > false if the input value is not an exact integer, whereas in fact exact_real_truncate returns false if the > truncation operation was not exact, which are different things. This would lead the function to accept any > CONST_DOUBLE that can truncate to a power of 2, such as 4.9, 16.2 etc. > > In any case, I've rewritten this function and used the real_isinteger predicate to check if the real value > is an exact integer. > > The testcase demonstrates the kind of wrong code that this patch addresses. > > This bug appears on GCC 5 and 4.9 as well, but due to the recent introduction of CONST_DOUBLE_REAL_VALUE > this patch doesn't apply on those branches. I will soon post the backportable variant. > > Bootstrapped and tested on arm-none-linux-gnueabihf. > > Ok for trunk? > > Thanks, > Kyrill > > 2015-10-12 Kyrylo Tkachov <kyrylo.tkachov@arm.com> > > PR target/67929 > * config/arm/arm.c (vfp3_const_double_for_bits): Rewrite. > * config/arm/constraints.md (Dp): Update callsite. > * config/arm/predicates.md (const_double_vcvt_power_of_two): Likewise. > > 2015-10-12 Kyrylo Tkachov <kyrylo.tkachov@arm.com> > > PR target/67929 > * gcc.target/arm/pr67929_1.c: New test.
Hi Kyrill, On 27 October 2015 at 13:08, Ramana Radhakrishnan <ramana.gcc@googlemail.com> wrote: > On Wed, Oct 14, 2015 at 1:23 PM, Kyrill Tkachov <kyrylo.tkachov@arm.com> wrote: >> Hi all, >> >> This patch fixes the referenced PR by rewriting the >> vfp3_const_double_for_bits function in arm.c >> The function is supposed to accept positive CONST_DOUBLE rtxes whose value >> is an exact power of 2 >> and whose log2 is between 1 and 32. That is values like 2.0, 4.0, 8.9, 16.0 >> etc... >> >> The current implementation seems to have been written under the assumption >> that exact_real_truncate returns >> false if the input value is not an exact integer, whereas in fact >> exact_real_truncate returns false if the >> truncation operation was not exact, which are different things. This would >> lead the function to accept any >> CONST_DOUBLE that can truncate to a power of 2, such as 4.9, 16.2 etc. >> >> In any case, I've rewritten this function and used the real_isinteger >> predicate to check if the real value >> is an exact integer. >> >> The testcase demonstrates the kind of wrong code that this patch addresses. >> >> This bug appears on GCC 5 and 4.9 as well, but due to the recent >> introduction of CONST_DOUBLE_REAL_VALUE >> this patch doesn't apply on those branches. I will soon post the >> backportable variant. >> >> Bootstrapped and tested on arm-none-linux-gnueabihf. >> >> Ok for trunk? > > > Thanks, this is OK for trunk and all release branches. > > Ramana > >> >> Thanks, >> Kyrill >> >> 2015-10-12 Kyrylo Tkachov <kyrylo.tkachov@arm.com> >> >> PR target/67929 >> * config/arm/arm.c (vfp3_const_double_for_bits): Rewrite. >> * config/arm/constraints.md (Dp): Update callsite. >> * config/arm/predicates.md (const_double_vcvt_power_of_two): Likewise. >> >> 2015-10-12 Kyrylo Tkachov <kyrylo.tkachov@arm.com> >> >> PR target/67929 >> * gcc.target/arm/pr67929_1.c: New test. This test fails when tested on hard-float targets, adding the following line to avoid testing it in such cases will fix the issue, but I wonder if there is a better dejaGNU directives sequence to do that. /* { dg-skip-if "avoid conflicting multilib options" { *-*-*eabihf } { "*" } { "" } } */ Cheers, Yvan
On 2 November 2015 at 09:38, Ramana Radhakrishnan <ramana.radhakrishnan@foss.arm.com> wrote: > >>>> >>>> 2015-10-12 Kyrylo Tkachov <kyrylo.tkachov@arm.com> >>>> >>>> PR target/67929 >>>> * gcc.target/arm/pr67929_1.c: New test. >> >> This test fails when tested on hard-float targets, adding the >> following line to avoid testing it in such cases will fix the issue, >> but I wonder if there is a better dejaGNU directives sequence to do >> that. >> >> /* { dg-skip-if "avoid conflicting multilib options" { *-*-*eabihf } { >> "*" } { "" } } */ > > No, not without further investigation into why the test is failing. Sorry, it fails because of the ABI mismatch between the built libs for HF targets and the testcase which is built with the flag -mfloat-abi=softfp (which is added by the directive arm_vfpv3_ok) Yvan > regards > Ramana > >> >> Cheers, >> Yvan >>
On 2 November 2015 at 09:51, Yvan Roux <yvan.roux@linaro.org> wrote: > On 2 November 2015 at 09:38, Ramana Radhakrishnan > <ramana.radhakrishnan@foss.arm.com> wrote: >> >>>>> >>>>> 2015-10-12 Kyrylo Tkachov <kyrylo.tkachov@arm.com> >>>>> >>>>> PR target/67929 >>>>> * gcc.target/arm/pr67929_1.c: New test. >>> >>> This test fails when tested on hard-float targets, adding the >>> following line to avoid testing it in such cases will fix the issue, >>> but I wonder if there is a better dejaGNU directives sequence to do >>> that. >>> >>> /* { dg-skip-if "avoid conflicting multilib options" { *-*-*eabihf } { >>> "*" } { "" } } */ >> >> No, not without further investigation into why the test is failing. > > Sorry, it fails because of the ABI mismatch between the built libs for > HF targets and the testcase which is built with the flag > -mfloat-abi=softfp (which is added by the directive arm_vfpv3_ok) > I think that's what I meant in: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67929#c7 Christophe. > Yvan > >> regards >> Ramana >> >>> >>> Cheers, >>> Yvan >>>
On 02/11/15 08:38, Ramana Radhakrishnan wrote: >>>> 2015-10-12 Kyrylo Tkachov <kyrylo.tkachov@arm.com> >>>> >>>> PR target/67929 >>>> * gcc.target/arm/pr67929_1.c: New test. >> This test fails when tested on hard-float targets, adding the >> following line to avoid testing it in such cases will fix the issue, >> but I wonder if there is a better dejaGNU directives sequence to do >> that. >> >> /* { dg-skip-if "avoid conflicting multilib options" { *-*-*eabihf } { >> "*" } { "" } } */ > No, not without further investigation into why the test is failing. I believe it's the same issue that Christophe reported in the comment for https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67929. It's because /* { dg-add-options arm_vfp3 } */ added -mfloat-abi=softfp but the crt* files were compiled for hardfp. It's a testism. I think adding an explicit -mfloat-abi=hard and gating on arm_hard_vfp_ok? > regards > Ramana > >> Cheers, >> Yvan >>
On 2 November 2015 at 10:24, Ramana Radhakrishnan <ramana.radhakrishnan@foss.arm.com> wrote: > > > On 02/11/15 09:01, Christophe Lyon wrote: >> On 2 November 2015 at 09:51, Yvan Roux <yvan.roux@linaro.org> wrote: >>> On 2 November 2015 at 09:38, Ramana Radhakrishnan >>> <ramana.radhakrishnan@foss.arm.com> wrote: >>>> >>>>>>> >>>>>>> 2015-10-12 Kyrylo Tkachov <kyrylo.tkachov@arm.com> >>>>>>> >>>>>>> PR target/67929 >>>>>>> * gcc.target/arm/pr67929_1.c: New test. >>>>> >>>>> This test fails when tested on hard-float targets, adding the >>>>> following line to avoid testing it in such cases will fix the issue, >>>>> but I wonder if there is a better dejaGNU directives sequence to do >>>>> that. >>>>> >>>>> /* { dg-skip-if "avoid conflicting multilib options" { *-*-*eabihf } { >>>>> "*" } { "" } } */ >>>> >>>> No, not without further investigation into why the test is failing. >>> >>> Sorry, it fails because of the ABI mismatch between the built libs for >>> HF targets and the testcase which is built with the flag >>> -mfloat-abi=softfp (which is added by the directive arm_vfpv3_ok) >>> >> I think that's what I meant in: >> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67929#c7 > > Ah, I see what you mean - instead I would just remove all the special options and move this test into gcc.c-torture/execute. > > There are enough testers that test by default to armhf now for us to be worried about testing the exact combination. Ha yes that's ture and I remember that we ended to that same conclusion for one testcase I tried to find the exact float ABI flag combination several months ago. Yvan > regards > Ramana > >> >> Christophe. >> >>> Yvan >>> >>>> regards >>>> Ramana >>>> >>>>> >>>>> Cheers, >>>>> Yvan >>>>>
On 02/11/15 09:28, Yvan Roux wrote: > On 2 November 2015 at 10:24, Ramana Radhakrishnan > <ramana.radhakrishnan@foss.arm.com> wrote: >> >> On 02/11/15 09:01, Christophe Lyon wrote: >>> On 2 November 2015 at 09:51, Yvan Roux <yvan.roux@linaro.org> wrote: >>>> On 2 November 2015 at 09:38, Ramana Radhakrishnan >>>> <ramana.radhakrishnan@foss.arm.com> wrote: >>>>>>>> 2015-10-12 Kyrylo Tkachov <kyrylo.tkachov@arm.com> >>>>>>>> >>>>>>>> PR target/67929 >>>>>>>> * gcc.target/arm/pr67929_1.c: New test. >>>>>> This test fails when tested on hard-float targets, adding the >>>>>> following line to avoid testing it in such cases will fix the issue, >>>>>> but I wonder if there is a better dejaGNU directives sequence to do >>>>>> that. >>>>>> >>>>>> /* { dg-skip-if "avoid conflicting multilib options" { *-*-*eabihf } { >>>>>> "*" } { "" } } */ >>>>> No, not without further investigation into why the test is failing. >>>> Sorry, it fails because of the ABI mismatch between the built libs for >>>> HF targets and the testcase which is built with the flag >>>> -mfloat-abi=softfp (which is added by the directive arm_vfpv3_ok) >>>> >>> I think that's what I meant in: >>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67929#c7 >> Ah, I see what you mean - instead I would just remove all the special options and move this test into gcc.c-torture/execute. >> >> There are enough testers that test by default to armhf now for us to be worried about testing the exact combination. > Ha yes that's ture and I remember that we ended to that same > conclusion for one testcase I tried to find the exact float ABI flag > combination several months ago. Ok, moving the test to the torture suite sounds best. I'll prepare a patch. Sorry for the trouble, Kyrill > > > Yvan >> regards >> Ramana >> >>> Christophe. >>> >>>> Yvan >>>> >>>>> regards >>>>> Ramana >>>>> >>>>>> Cheers, >>>>>> Yvan >>>>>>
diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c index 0bf1164..29dd489 100644 --- a/gcc/config/arm/arm.c +++ b/gcc/config/arm/arm.c @@ -27734,25 +27734,37 @@ vfp3_const_double_for_fract_bits (rtx operand) return 0; } +/* If X is a CONST_DOUBLE with a value that is a power of 2 whose + log2 is in [1, 32], return that log2. Otherwise return -1. + This is used in the patterns for vcvt.s32.f32 floating-point to + fixed-point conversions. */ + int -vfp3_const_double_for_bits (rtx operand) +vfp3_const_double_for_bits (rtx x) { - const REAL_VALUE_TYPE *r0; + const REAL_VALUE_TYPE *r; - if (!CONST_DOUBLE_P (operand)) - return 0; + if (!CONST_DOUBLE_P (x)) + return -1; - r0 = CONST_DOUBLE_REAL_VALUE (operand); - if (exact_real_truncate (DFmode, r0)) - { - HOST_WIDE_INT value = real_to_integer (r0); - value = value & 0xffffffff; - if ((value != 0) && ( (value & (value - 1)) == 0)) - return int_log2 (value); - } + r = CONST_DOUBLE_REAL_VALUE (x); - return 0; + if (REAL_VALUE_NEGATIVE (*r) + || REAL_VALUE_ISNAN (*r) + || REAL_VALUE_ISINF (*r) + || !real_isinteger (r, SFmode)) + return -1; + + HOST_WIDE_INT hwint = exact_log2 (real_to_integer (r)); + +/* The exact_log2 above will have returned -1 if this is + not an exact log2. */ + if (!IN_RANGE (hwint, 1, 32)) + return -1; + + return hwint; } + /* Emit a memory barrier around an atomic sequence according to MODEL. */ diff --git a/gcc/config/arm/constraints.md b/gcc/config/arm/constraints.md index e24858f..901cfe5 100644 --- a/gcc/config/arm/constraints.md +++ b/gcc/config/arm/constraints.md @@ -339,7 +339,8 @@ "@internal In ARM/ Thumb2 a const_double which can be used with a vcvt.s32.f32 with bits operation" (and (match_code "const_double") - (match_test "TARGET_32BIT && TARGET_VFP && vfp3_const_double_for_bits (op)"))) + (match_test "TARGET_32BIT && TARGET_VFP + && vfp3_const_double_for_bits (op) > 0"))) (define_register_constraint "Ts" "(arm_restrict_it) ? LO_REGS : GENERAL_REGS" "For arm_restrict_it the core registers @code{r0}-@code{r7}. GENERAL_REGS otherwise.") diff --git a/gcc/config/arm/predicates.md b/gcc/config/arm/predicates.md index 08cc899..48e4ba8 100644 --- a/gcc/config/arm/predicates.md +++ b/gcc/config/arm/predicates.md @@ -668,7 +668,7 @@ (define_predicate "const_double_vcvt_power_of_two" (and (match_code "const_double") (match_test "TARGET_32BIT && TARGET_VFP - && vfp3_const_double_for_bits (op)"))) + && vfp3_const_double_for_bits (op) > 0"))) (define_predicate "neon_struct_operand" (and (match_code "mem") diff --git a/gcc/testsuite/gcc.target/arm/pr67929_1.c b/gcc/testsuite/gcc.target/arm/pr67929_1.c new file mode 100644 index 0000000..14943b6 --- /dev/null +++ b/gcc/testsuite/gcc.target/arm/pr67929_1.c @@ -0,0 +1,21 @@ +/* { dg-do run } */ +/* { dg-require-effective-target arm_vfp3_ok } */ +/* { dg-options "-O2 -fno-inline" } */ +/* { dg-add-options arm_vfp3 } */ +/* { dg-skip-if "need fp instructions" { *-*-* } { "-mfloat-abi=soft" } { "" } } */ + +int +foo (float a) +{ + return a * 4.9f; +} + + +int +main (void) +{ + if (foo (10.0f) != 49) + __builtin_abort (); + + return 0; +} \ No newline at end of file
Hi all, This patch fixes the referenced PR by rewriting the vfp3_const_double_for_bits function in arm.c The function is supposed to accept positive CONST_DOUBLE rtxes whose value is an exact power of 2 and whose log2 is between 1 and 32. That is values like 2.0, 4.0, 8.9, 16.0 etc... The current implementation seems to have been written under the assumption that exact_real_truncate returns false if the input value is not an exact integer, whereas in fact exact_real_truncate returns false if the truncation operation was not exact, which are different things. This would lead the function to accept any CONST_DOUBLE that can truncate to a power of 2, such as 4.9, 16.2 etc. In any case, I've rewritten this function and used the real_isinteger predicate to check if the real value is an exact integer. The testcase demonstrates the kind of wrong code that this patch addresses. This bug appears on GCC 5 and 4.9 as well, but due to the recent introduction of CONST_DOUBLE_REAL_VALUE this patch doesn't apply on those branches. I will soon post the backportable variant. Bootstrapped and tested on arm-none-linux-gnueabihf. Ok for trunk? Thanks, Kyrill 2015-10-12 Kyrylo Tkachov <kyrylo.tkachov@arm.com> PR target/67929 * config/arm/arm.c (vfp3_const_double_for_bits): Rewrite. * config/arm/constraints.md (Dp): Update callsite. * config/arm/predicates.md (const_double_vcvt_power_of_two): Likewise. 2015-10-12 Kyrylo Tkachov <kyrylo.tkachov@arm.com> PR target/67929 * gcc.target/arm/pr67929_1.c: New test.