[PATCH][ARM] PR target/67929 Tighten vfp3_const_double_for_bits checks

[PATCH][ARM] PR target/67929 Tighten vfp3_const_double_for_bits checks

[Kyrill Tkachov]

[-- Attachment #1: Type: text/plain, Size: 1594 bytes --]

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.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: arm-vmul-vcvt.patch --]
[-- Type: text/x-patch; name=arm-vmul-vcvt.patch, Size: 3414 bytes --]

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;
 }
+
 \f
 /* 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

Re: [PATCH][ARM] PR target/67929 Tighten vfp3_const_double_for_bits checks

[Kyrill Tkachov]

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.

Re: [PATCH][ARM] PR target/67929 Tighten vfp3_const_double_for_bits checks

[Ramana Radhakrishnan]

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.

Re: [PATCH][ARM] PR target/67929 Tighten vfp3_const_double_for_bits checks

[Yvan Roux]

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

Re: [PATCH][ARM] PR target/67929 Tighten vfp3_const_double_for_bits checks

[Ramana Radhakrishnan]

>>>
>>> 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.

regards
Ramana

> 
> Cheers,
> Yvan
> 

Re: [PATCH][ARM] PR target/67929 Tighten vfp3_const_double_for_bits checks

[Yvan Roux]

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
>>

Re: [PATCH][ARM] PR target/67929 Tighten vfp3_const_double_for_bits checks

[Christophe Lyon]

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
>>>

Re: [PATCH][ARM] PR target/67929 Tighten vfp3_const_double_for_bits checks

[Kyrill Tkachov]

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
>>

Re: [PATCH][ARM] PR target/67929 Tighten vfp3_const_double_for_bits checks

[Ramana Radhakrishnan]

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.

regards
Ramana

> 
> Christophe.
> 
>> Yvan
>>
>>> regards
>>> Ramana
>>>
>>>>
>>>> Cheers,
>>>> Yvan
>>>>

Re: [PATCH][ARM] PR target/67929 Tighten vfp3_const_double_for_bits checks

[Ramana Radhakrishnan]

On 02/11/15 09:20, Kyrill Tkachov wrote:
> 
> 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?

No, see my other response. 

Looking at it again, I can't see anything ARM specific in the source
that causes this to remain in gcc.target/arm.

Instead remove all the special casing and move it into gcc.c-torture/execute.

regards
Ramana

> 
>> regards
>> Ramana
>>
>>> Cheers,
>>> Yvan
>>>
> 

Re: [PATCH][ARM] PR target/67929 Tighten vfp3_const_double_for_bits checks

[Yvan Roux]

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
>>>>>

Re: [PATCH][ARM] PR target/67929 Tighten vfp3_const_double_for_bits checks

[Kyrill Tkachov]

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
>>>>>>

Re: [PATCH][ARM] PR target/67929 Tighten vfp3_const_double_for_bits checks

[Richard Earnshaw]

On 02/11/15 09:26, Ramana Radhakrishnan wrote:
> 
> 
> On 02/11/15 09:20, Kyrill Tkachov wrote:
>>
>> 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?
> 
> No, see my other response. 
> 
> Looking at it again, I can't see anything ARM specific in the source
> that causes this to remain in gcc.target/arm.
> 
> Instead remove all the special casing and move it into gcc.c-torture/execute.
> 

Agreed, but foo() should be marked no-inline, no-clone if doing that.

R.

> regards
> Ramana
> 
>>
>>> regards
>>> Ramana
>>>
>>>> Cheers,
>>>> Yvan
>>>>
>>

Re: [PATCH][ARM] PR target/67929 Tighten vfp3_const_double_for_bits checks

[Kyrill Tkachov]

[-- Attachment #1: Type: text/plain, Size: 2481 bytes --]


On 02/11/15 09:29, Kyrill Tkachov wrote:
>
> 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.
>

Is this proposed patch ok to commit?
It moves the test and adds noclone and noinline attributes to 'foo' like Richard suggested.

2015-11-01  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

     PR target/67929
     * gcc.target/arm/pr67929_1.c: Move to...
     * gcc.c-torture/execute/pr67929_1.c: ... Here.
     Remove arm-specific directives.  Add noclone, noinline
     attributes.

Thanks,
Kyrill

> Sorry for the trouble,
> Kyrill
>
>>
>>
>> Yvan
>>> regards
>>> Ramana
>>>
>>>> Christophe.
>>>>
>>>>> Yvan
>>>>>
>>>>>> regards
>>>>>> Ramana
>>>>>>
>>>>>>> Cheers,
>>>>>>> Yvan
>>>>>>>
>


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: arm-move-test.patch --]
[-- Type: text/x-patch; name=arm-move-test.patch, Size: 1288 bytes --]

commit f7da7437733ea999c09806c593d9b253fd2ba324
Author: Kyrylo Tkachov <kyrylo.tkachov@arm.com>
Date:   Mon Nov 2 11:16:57 2015 +0000

    Move gcc.target/arm/pr67929_1.c test to execute.exp

diff --git a/gcc/testsuite/gcc.c-torture/execute/pr67929_1.c b/gcc/testsuite/gcc.c-torture/execute/pr67929_1.c
new file mode 100644
index 0000000..ae6cfbf
--- /dev/null
+++ b/gcc/testsuite/gcc.c-torture/execute/pr67929_1.c
@@ -0,0 +1,15 @@
+int __attribute__ ((noinline, noclone))
+foo (float a)
+{
+  return a * 4.9f;
+}
+
+
+int
+main (void)
+{
+  if (foo (10.0f) != 49)
+    __builtin_abort ();
+
+  return 0;
+}
diff --git a/gcc/testsuite/gcc.target/arm/pr67929_1.c b/gcc/testsuite/gcc.target/arm/pr67929_1.c
deleted file mode 100644
index 14943b6..0000000
--- a/gcc/testsuite/gcc.target/arm/pr67929_1.c
+++ /dev/null
@@ -1,21 +0,0 @@
-/* { 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

Re: [PATCH][ARM] PR target/67929 Tighten vfp3_const_double_for_bits checks

[Richard Earnshaw]

On 02/11/15 11:36, Kyrill Tkachov wrote:
> 
> On 02/11/15 09:29, Kyrill Tkachov wrote:
>>
>> 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.
>>
> 
> Is this proposed patch ok to commit?
> It moves the test and adds noclone and noinline attributes to 'foo' like
> Richard suggested.
> 
> 2015-11-01  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
> 
>     PR target/67929
>     * gcc.target/arm/pr67929_1.c: Move to...
>     * gcc.c-torture/execute/pr67929_1.c: ... Here.
>     Remove arm-specific directives.  Add noclone, noinline
>     attributes.
> 

OK.

R.

> Thanks,
> Kyrill
> 
>> Sorry for the trouble,
>> Kyrill
>>
>>>
>>>
>>> Yvan
>>>> regards
>>>> Ramana
>>>>
>>>>> Christophe.
>>>>>
>>>>>> Yvan
>>>>>>
>>>>>>> regards
>>>>>>> Ramana
>>>>>>>
>>>>>>>> Cheers,
>>>>>>>> Yvan
>>>>>>>>
>>
> 
> 
> arm-move-test.patch
> 
> 
> commit f7da7437733ea999c09806c593d9b253fd2ba324
> Author: Kyrylo Tkachov <kyrylo.tkachov@arm.com>
> Date:   Mon Nov 2 11:16:57 2015 +0000
> 
>     Move gcc.target/arm/pr67929_1.c test to execute.exp
> 
> diff --git a/gcc/testsuite/gcc.c-torture/execute/pr67929_1.c b/gcc/testsuite/gcc.c-torture/execute/pr67929_1.c
> new file mode 100644
> index 0000000..ae6cfbf
> --- /dev/null
> +++ b/gcc/testsuite/gcc.c-torture/execute/pr67929_1.c
> @@ -0,0 +1,15 @@
> +int __attribute__ ((noinline, noclone))
> +foo (float a)
> +{
> +  return a * 4.9f;
> +}
> +
> +
> +int
> +main (void)
> +{
> +  if (foo (10.0f) != 49)
> +    __builtin_abort ();
> +
> +  return 0;
> +}
> diff --git a/gcc/testsuite/gcc.target/arm/pr67929_1.c b/gcc/testsuite/gcc.target/arm/pr67929_1.c
> deleted file mode 100644
> index 14943b6..0000000
> --- a/gcc/testsuite/gcc.target/arm/pr67929_1.c
> +++ /dev/null
> @@ -1,21 +0,0 @@
> -/* { 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
>