diff mbox

[ARM] : Fix static interworking call

Message ID 55FC21D0.909@st.com
State New
Headers show

Commit Message

Christian Bruel Sept. 18, 2015, 2:38 p.m. UTC 
On 09/18/2015 04:16 PM, Richard Earnshaw wrote:
> On 17/09/15 09:46, Christian Bruel wrote:
>> As obvious, bad operand number.
>>
>> OK for trunk ?
>>
>> Christian
>>
>>
>> p1.patch
>>
>>
>> 2015-09-18  Christian Bruel  <christian.bruel@st.com>
>>
>> 	* config/arm/arm.md (*call_value_symbol): Fix operand for interworking.
>>
>> 2015-09-18  Christian Bruel  <christian.bruel@st.com>
>>
>> 	* gcc.target/arm/attr_thumb-static2.c: New test.
>>
>> --- gnu_trunk.ref/gcc/gcc/config/arm/arm.md	2015-09-14 09:52:37.697264500 +0200
>> +++ gnu_trunk.p0/gcc/gcc/config/arm/arm.md	2015-09-17 10:03:33.849451705 +0200
>> @@ -7891,7 +7891,7 @@
>>      /* Switch mode now when possible.  */
>>      if (SYMBOL_REF_DECL (op) && !TREE_PUBLIC (SYMBOL_REF_DECL (op))
>>           && arm_arch5 && arm_change_mode_p (SYMBOL_REF_DECL (op)))
>> -      return NEED_PLT_RELOC ? \"blx%?\\t%a0(PLT)\" : \"blx%?\\t(%a0)\";
>> +      return NEED_PLT_RELOC ? \"blx%?\\t%a1(PLT)\" : \"blx%?\\t(%a1)\";
>>
>>       return NEED_PLT_RELOC ? \"bl%?\\t%a1(PLT)\" : \"bl%?\\t%a1\";
>>     }"
>> diff -ruNp gnu_trunk.ref/gcc/gcc/testsuite/gcc.target/arm/attr_thumb-static2.c gnu_trunk.p0/gcc/gcc/testsuite/gcc.target/arm/attr_thumb-static2.c
>> --- gnu_trunk.ref/gcc/gcc/testsuite/gcc.target/arm/attr_thumb-static2.c	1970-01-01 01:00:00.000000000 +0100
>> +++ gnu_trunk.p0/gcc/gcc/testsuite/gcc.target/arm/attr_thumb-static2.c	2015-09-17 10:08:08.350064131 +0200
>> @@ -0,0 +1,40 @@
>> +/* Check that interwork between static functions is correctly resolved. */
>> +
>> +/* { dg-skip-if "" { ! { arm_thumb1_ok || arm_thumb2_ok } } } */
>> +/* { dg-options "-O0 -march=armv7-a -mfloat-abi=hard" } */
>
> You can't have thumb1 and hard float,

Ah OK I didn't know that. Is it that there was no FPU before V5 ?

 > so the skip unless thumb1 seems a nonsense.

And there is no thumb1 and march=armv7-a !. So indeed the skip unless 
thumb1 is a nonsense.
Is the attached patch OK to clean this up ?

thanks,


>
> R.
>
>> +/* { dg-do compile } */
>> +
>> +struct _NSPoint
>> +{
>> +  float x;
>> +  float y;
>> +};
>> +
>> +typedef struct _NSPoint NSPoint;
>> +
>> +static NSPoint
>> +__attribute__ ((target("arm")))
>> +NSMakePoint (float x, float y)
>> +{
>> +  NSPoint point;
>> +  point.x = x;
>> +  point.y = y;
>> +  return point;
>> +}
>> +
>> +static NSPoint
>> +__attribute__ ((target("thumb")))
>> +RelativePoint (NSPoint point, NSPoint refPoint)
>> +{
>> +  return NSMakePoint (refPoint.x + point.x, refPoint.y + point.y);
>> +}
>> +
>> +NSPoint
>> +__attribute__ ((target("arm")))
>> +g(NSPoint refPoint)
>> +{
>> +  float pointA, pointB;
>> +  return RelativePoint (NSMakePoint (0, pointA), refPoint);
>> +}
>> +
>> +/* { dg-final { scan-assembler-times "blx" 2 } } */
>>
>

Comments

Richard Earnshaw Sept. 18, 2015, 3:03 p.m. UTC | #1 
On 18/09/15 15:38, Christian Bruel wrote:
> 
> 
> On 09/18/2015 04:16 PM, Richard Earnshaw wrote:
>> On 17/09/15 09:46, Christian Bruel wrote:
>>> As obvious, bad operand number.
>>>
>>> OK for trunk ?
>>>
>>> Christian
>>>
>>>
>>> p1.patch
>>>
>>>
>>> 2015-09-18  Christian Bruel  <christian.bruel@st.com>
>>>
>>>     * config/arm/arm.md (*call_value_symbol): Fix operand for
>>> interworking.
>>>
>>> 2015-09-18  Christian Bruel  <christian.bruel@st.com>
>>>
>>>     * gcc.target/arm/attr_thumb-static2.c: New test.
>>>
>>> --- gnu_trunk.ref/gcc/gcc/config/arm/arm.md    2015-09-14
>>> 09:52:37.697264500 +0200
>>> +++ gnu_trunk.p0/gcc/gcc/config/arm/arm.md    2015-09-17
>>> 10:03:33.849451705 +0200
>>> @@ -7891,7 +7891,7 @@
>>>      /* Switch mode now when possible.  */
>>>      if (SYMBOL_REF_DECL (op) && !TREE_PUBLIC (SYMBOL_REF_DECL (op))
>>>           && arm_arch5 && arm_change_mode_p (SYMBOL_REF_DECL (op)))
>>> -      return NEED_PLT_RELOC ? \"blx%?\\t%a0(PLT)\" : \"blx%?\\t(%a0)\";
>>> +      return NEED_PLT_RELOC ? \"blx%?\\t%a1(PLT)\" : \"blx%?\\t(%a1)\";
>>>
>>>       return NEED_PLT_RELOC ? \"bl%?\\t%a1(PLT)\" : \"bl%?\\t%a1\";
>>>     }"
>>> diff -ruNp
>>> gnu_trunk.ref/gcc/gcc/testsuite/gcc.target/arm/attr_thumb-static2.c
>>> gnu_trunk.p0/gcc/gcc/testsuite/gcc.target/arm/attr_thumb-static2.c
>>> ---
>>> gnu_trunk.ref/gcc/gcc/testsuite/gcc.target/arm/attr_thumb-static2.c    1970-01-01
>>> 01:00:00.000000000 +0100
>>> +++
>>> gnu_trunk.p0/gcc/gcc/testsuite/gcc.target/arm/attr_thumb-static2.c   
>>> 2015-09-17 10:08:08.350064131 +0200
>>> @@ -0,0 +1,40 @@
>>> +/* Check that interwork between static functions is correctly
>>> resolved. */
>>> +
>>> +/* { dg-skip-if "" { ! { arm_thumb1_ok || arm_thumb2_ok } } } */
>>> +/* { dg-options "-O0 -march=armv7-a -mfloat-abi=hard" } */
>>
>> You can't have thumb1 and hard float,
> 
> Ah OK I didn't know that. Is it that there was no FPU before V5 ?
> 

Thumb1 had no instruction encodings for accessing the FPU.

>> so the skip unless thumb1 seems a nonsense.
> 
> And there is no thumb1 and march=armv7-a !. So indeed the skip unless
> thumb1 is a nonsense.
> Is the attached patch OK to clean this up ?
> 
> thanks,
> 
> 
>>
>> R.
>>
>>> +/* { dg-do compile } */
>>> +
>>> +struct _NSPoint
>>> +{
>>> +  float x;
>>> +  float y;
>>> +};
>>> +
>>> +typedef struct _NSPoint NSPoint;
>>> +
>>> +static NSPoint
>>> +__attribute__ ((target("arm")))
>>> +NSMakePoint (float x, float y)
>>> +{
>>> +  NSPoint point;
>>> +  point.x = x;
>>> +  point.y = y;
>>> +  return point;
>>> +}
>>> +
>>> +static NSPoint
>>> +__attribute__ ((target("thumb")))
>>> +RelativePoint (NSPoint point, NSPoint refPoint)
>>> +{
>>> +  return NSMakePoint (refPoint.x + point.x, refPoint.y + point.y);
>>> +}
>>> +
>>> +NSPoint
>>> +__attribute__ ((target("arm")))
>>> +g(NSPoint refPoint)
>>> +{
>>> +  float pointA, pointB;
>>> +  return RelativePoint (NSMakePoint (0, pointA), refPoint);
>>> +}
>>> +
>>> +/* { dg-final { scan-assembler-times "blx" 2 } } */
>>>
>>
> 
> 1.patch
> 
> 
> 2015-09-17  Christian Bruel  <christian.bruel@st.com>
> 
> 	* gcc.target/arm/attr_thumb-static2.c: Test only for thumb2.
> 
> Index: attr_thumb-static2.c
> ===================================================================
> --- attr_thumb-static2.c	(revision 227904)
> +++ attr_thumb-static2.c	(working copy)
> @@ -1,6 +1,6 @@
>  /* Check that interwork between static functions is correctly resolved. */
>  
> -/* { dg-skip-if "" { ! { arm_thumb1_ok || arm_thumb2_ok } } } */
> +/* { dg-require-effective-target arm_thumb2_ok } */
>  /* { dg-options "-O0 -march=armv7-a -mfloat-abi=hard" } */
>  /* { dg-do compile } */
>  
> 

Do you really need -mfloat-abi=hard for this test?  If so, I think you
also need "dg-require-effective-target arm_hard_vfp_ok".  See
gcc.target/arm/pr65729.c

R.
Christophe Lyon Sept. 20, 2015, 11:55 p.m. UTC | #2 
On 18 September 2015 at 17:03, Richard Earnshaw
<Richard.Earnshaw@foss.arm.com> wrote:
> On 18/09/15 15:38, Christian Bruel wrote:
>>
>>
>> On 09/18/2015 04:16 PM, Richard Earnshaw wrote:
>>> On 17/09/15 09:46, Christian Bruel wrote:
>>>> As obvious, bad operand number.
>>>>
>>>> OK for trunk ?
>>>>
>>>> Christian
>>>>
>>>>
>>>> p1.patch
>>>>
>>>>
>>>> 2015-09-18  Christian Bruel  <christian.bruel@st.com>
>>>>
>>>>     * config/arm/arm.md (*call_value_symbol): Fix operand for
>>>> interworking.
>>>>
>>>> 2015-09-18  Christian Bruel  <christian.bruel@st.com>
>>>>
>>>>     * gcc.target/arm/attr_thumb-static2.c: New test.
>>>>
>>>> --- gnu_trunk.ref/gcc/gcc/config/arm/arm.md    2015-09-14
>>>> 09:52:37.697264500 +0200
>>>> +++ gnu_trunk.p0/gcc/gcc/config/arm/arm.md    2015-09-17
>>>> 10:03:33.849451705 +0200
>>>> @@ -7891,7 +7891,7 @@
>>>>      /* Switch mode now when possible.  */
>>>>      if (SYMBOL_REF_DECL (op) && !TREE_PUBLIC (SYMBOL_REF_DECL (op))
>>>>           && arm_arch5 && arm_change_mode_p (SYMBOL_REF_DECL (op)))
>>>> -      return NEED_PLT_RELOC ? \"blx%?\\t%a0(PLT)\" : \"blx%?\\t(%a0)\";
>>>> +      return NEED_PLT_RELOC ? \"blx%?\\t%a1(PLT)\" : \"blx%?\\t(%a1)\";
>>>>
>>>>       return NEED_PLT_RELOC ? \"bl%?\\t%a1(PLT)\" : \"bl%?\\t%a1\";
>>>>     }"
>>>> diff -ruNp
>>>> gnu_trunk.ref/gcc/gcc/testsuite/gcc.target/arm/attr_thumb-static2.c
>>>> gnu_trunk.p0/gcc/gcc/testsuite/gcc.target/arm/attr_thumb-static2.c
>>>> ---
>>>> gnu_trunk.ref/gcc/gcc/testsuite/gcc.target/arm/attr_thumb-static2.c    1970-01-01
>>>> 01:00:00.000000000 +0100
>>>> +++
>>>> gnu_trunk.p0/gcc/gcc/testsuite/gcc.target/arm/attr_thumb-static2.c
>>>> 2015-09-17 10:08:08.350064131 +0200
>>>> @@ -0,0 +1,40 @@
>>>> +/* Check that interwork between static functions is correctly
>>>> resolved. */
>>>> +
>>>> +/* { dg-skip-if "" { ! { arm_thumb1_ok || arm_thumb2_ok } } } */
>>>> +/* { dg-options "-O0 -march=armv7-a -mfloat-abi=hard" } */
>>>
>>> You can't have thumb1 and hard float,
>>
>> Ah OK I didn't know that. Is it that there was no FPU before V5 ?
>>
>
> Thumb1 had no instruction encodings for accessing the FPU.
>
>>> so the skip unless thumb1 seems a nonsense.
>>
>> And there is no thumb1 and march=armv7-a !. So indeed the skip unless
>> thumb1 is a nonsense.
>> Is the attached patch OK to clean this up ?
>>
>> thanks,
>>
>>
>>>
>>> R.
>>>
>>>> +/* { dg-do compile } */
>>>> +
>>>> +struct _NSPoint
>>>> +{
>>>> +  float x;
>>>> +  float y;
>>>> +};
>>>> +
>>>> +typedef struct _NSPoint NSPoint;
>>>> +
>>>> +static NSPoint
>>>> +__attribute__ ((target("arm")))
>>>> +NSMakePoint (float x, float y)
>>>> +{
>>>> +  NSPoint point;
>>>> +  point.x = x;
>>>> +  point.y = y;
>>>> +  return point;
>>>> +}
>>>> +
>>>> +static NSPoint
>>>> +__attribute__ ((target("thumb")))
>>>> +RelativePoint (NSPoint point, NSPoint refPoint)
>>>> +{
>>>> +  return NSMakePoint (refPoint.x + point.x, refPoint.y + point.y);
>>>> +}
>>>> +
>>>> +NSPoint
>>>> +__attribute__ ((target("arm")))
>>>> +g(NSPoint refPoint)
>>>> +{
>>>> +  float pointA, pointB;
>>>> +  return RelativePoint (NSMakePoint (0, pointA), refPoint);
>>>> +}
>>>> +
>>>> +/* { dg-final { scan-assembler-times "blx" 2 } } */
>>>>
>>>
>>
>> 1.patch
>>
>>
>> 2015-09-17  Christian Bruel  <christian.bruel@st.com>
>>
>>       * gcc.target/arm/attr_thumb-static2.c: Test only for thumb2.
>>
>> Index: attr_thumb-static2.c
>> ===================================================================
>> --- attr_thumb-static2.c      (revision 227904)
>> +++ attr_thumb-static2.c      (working copy)
>> @@ -1,6 +1,6 @@
>>  /* Check that interwork between static functions is correctly resolved. */
>>
>> -/* { dg-skip-if "" { ! { arm_thumb1_ok || arm_thumb2_ok } } } */
>> +/* { dg-require-effective-target arm_thumb2_ok } */
>>  /* { dg-options "-O0 -march=armv7-a -mfloat-abi=hard" } */
>>  /* { dg-do compile } */
>>
>>
>
> Do you really need -mfloat-abi=hard for this test?  If so, I think you
> also need "dg-require-effective-target arm_hard_vfp_ok".  See
> gcc.target/arm/pr65729.c
>
> R.
>
Christian,

It seems you committed the 1st version of your patch.
However, it fails if one forces a armv5t target, because, as Richard
said -mfloat-abi=hard is not supported on Thumb1.

It tried to remove -mfloat-abi=hard, and I noticed that only 1 'blx'
is generated when using soft or softfp.

I expect this to be fixed by the linker anyway, but it may mean there
is still something wrong.

Christophe.
Christian Bruel Sept. 21, 2015, 10:55 a.m. UTC | #3 
Hi Christophe,

> It seems you committed the 1st version of your patch.

Not sure what version you are talking about. I committed what was posted 
(https://gcc.gnu.org/ml/gcc-patches/2015-09/msg01260.html) at revision 
#227880.

> However, it fails if one forces a armv5t target, because, as Richard
> said -mfloat-abi=hard is not supported on Thumb1.

sure, but the testcase was for thumb2. It's just that the thumb1 check 
in the dg-skip-if is indeed useless. I'll fix this.

>
> It tried to remove -mfloat-abi=hard, and I noticed that only 1 'blx'
> is generated when using soft or softfp.
>
> I expect this to be fixed by the linker anyway, but it may mean there
> is still something wrong.

This is expected. For thumb1, such static calls interwork are still done 
by the linker.
For consistency IMHO we could also resolve static interwork thumb1 calls 
in the compiler, but that would be another issue.
This failure was only for thumb2 (this having -march=armv7-a in the 
dg-options). (See https://sourceware.org/bugzilla/show_bug.cgi?id=17505)

Hope that clarifies,

thanks

Christian
Christophe Lyon Sept. 21, 2015, 2 p.m. UTC | #4 
On 21 September 2015 at 12:55, Christian Bruel <christian.bruel@st.com> wrote:
> Hi Christophe,
>
>> It seems you committed the 1st version of your patch.
>
>
> Not sure what version you are talking about. I committed what was posted
> (https://gcc.gnu.org/ml/gcc-patches/2015-09/msg01260.html) at revision
> #227880.

I thought you had modified it, after Richard's comments.

>
>> However, it fails if one forces a armv5t target, because, as Richard
>> said -mfloat-abi=hard is not supported on Thumb1.
>
>
> sure, but the testcase was for thumb2. It's just that the thumb1 check in
> the dg-skip-if is indeed useless. I'll fix this.
>
>>
>> It tried to remove -mfloat-abi=hard, and I noticed that only 1 'blx'
>> is generated when using soft or softfp.
>>
>> I expect this to be fixed by the linker anyway, but it may mean there
>> is still something wrong.
>
>
> This is expected. For thumb1, such static calls interwork are still done by
> the linker.
> For consistency IMHO we could also resolve static interwork thumb1 calls in
> the compiler, but that would be another issue.
> This failure was only for thumb2 (this having -march=armv7-a in the
> dg-options). (See https://sourceware.org/bugzilla/show_bug.cgi?id=17505)
>
> Hope that clarifies,
>
Yes, thanks.

> thanks
>
> Christian
Christian Bruel Sept. 21, 2015, 2:15 p.m. UTC | #5 
On 09/18/2015 05:03 PM, Richard Earnshaw wrote:

>> Index: attr_thumb-static2.c
>> ===================================================================
>> --- attr_thumb-static2.c	(revision 227904)
>> +++ attr_thumb-static2.c	(working copy)
>> @@ -1,6 +1,6 @@
>>   /* Check that interwork between static functions is correctly resolved. */
>>
>> -/* { dg-skip-if "" { ! { arm_thumb1_ok || arm_thumb2_ok } } } */
>> +/* { dg-require-effective-target arm_thumb2_ok } */
>>   /* { dg-options "-O0 -march=armv7-a -mfloat-abi=hard" } */
>>   /* { dg-do compile } */
>>
>>
>
> Do you really need -mfloat-abi=hard for this test?  If so, I think you
> also need "dg-require-effective-target arm_hard_vfp_ok".  See
> gcc.target/arm/pr65729.c
>

The test was not crashing for -mfloat-abi=soft.
But the number of blx is independent. So yes we can write the conditions 
in such a way the test runs without hard fp.

is this one better ?
Christophe Lyon Sept. 23, 2015, 9:44 p.m. UTC | #6 
On 21 September 2015 at 16:15, Christian Bruel <christian.bruel@st.com> wrote:
>
>
> On 09/18/2015 05:03 PM, Richard Earnshaw wrote:
>
>>> Index: attr_thumb-static2.c
>>> ===================================================================
>>> --- attr_thumb-static2.c        (revision 227904)
>>> +++ attr_thumb-static2.c        (working copy)
>>> @@ -1,6 +1,6 @@
>>>   /* Check that interwork between static functions is correctly resolved.
>>> */
>>>
>>> -/* { dg-skip-if "" { ! { arm_thumb1_ok || arm_thumb2_ok } } } */
>>> +/* { dg-require-effective-target arm_thumb2_ok } */
>>>   /* { dg-options "-O0 -march=armv7-a -mfloat-abi=hard" } */
>>>   /* { dg-do compile } */
>>>
>>>
>>
>> Do you really need -mfloat-abi=hard for this test?  If so, I think you
>> also need "dg-require-effective-target arm_hard_vfp_ok".  See
>> gcc.target/arm/pr65729.c
>>
>
> The test was not crashing for -mfloat-abi=soft.
> But the number of blx is independent. So yes we can write the conditions in
> such a way the test runs without hard fp.
>
> is this one better ?

You need to move the dg-do directive before the other ones (otherwise,
it overrides the effect of require-target).
diff mbox

Patch

2015-09-17  Christian Bruel  <christian.bruel@st.com>

	* gcc.target/arm/attr_thumb-static2.c: Test only for thumb2.

Index: attr_thumb-static2.c
===================================================================
--- attr_thumb-static2.c	(revision 227904)
+++ attr_thumb-static2.c	(working copy)
@@ -1,6 +1,6 @@ 
 /* Check that interwork between static functions is correctly resolved. */
 
-/* { dg-skip-if "" { ! { arm_thumb1_ok || arm_thumb2_ok } } } */
+/* { dg-require-effective-target arm_thumb2_ok } */
 /* { dg-options "-O0 -march=armv7-a -mfloat-abi=hard" } */
 /* { dg-do compile } */