diff mbox

ARMv6-M MI thunk fix

Message ID 51B20F5A.5000908@codesourcery.com
State New
Headers show

Commit Message

Cesar Philippidis June 7, 2013, 4:50 p.m. UTC 
On 6/6/13 9:00 AM, Richard Earnshaw wrote:
> The pipeline offset is 4 for Thumb2 as well.  So at the very least you
> need to explain why your change doesn't apply then as well.

Yes some context is lost in that comment.  Thunks are usually emitted in
ARM mode, except for Thumb-only targets.  Is the new comment OK?  If so,
please check it in since I do not have SVN write access.

Thanks,
Cesar


2013-06-07  Julian Brown  <julian@codesourcery.com>
	    Cesar Philippidis  <cesar@codesourcery.com>

	gcc/
	* config/arm/arm.c (arm_output_mi_thunk): Fix offset for
	TARGET_THUMB1_ONLY.

Comments

Richard Earnshaw June 10, 2013, 3:32 p.m. UTC | #1 
On 07/06/13 17:50, Cesar Philippidis wrote:
> On 6/6/13 9:00 AM, Richard Earnshaw wrote:
>> The pipeline offset is 4 for Thumb2 as well.  So at the very least you
>> need to explain why your change doesn't apply then as well.
>
> Yes some context is lost in that comment.  Thunks are usually emitted in
> ARM mode, except for Thumb-only targets.  Is the new comment OK?  If so,
> please check it in since I do not have SVN write access.
>

So what about ARMv7-M, which is thumb2 but no ARM state?

R.

> Thanks,
> Cesar
>
>
> 2013-06-07  Julian Brown  <julian@codesourcery.com>
> 	    Cesar Philippidis  <cesar@codesourcery.com>
>
> 	gcc/
> 	* config/arm/arm.c (arm_output_mi_thunk): Fix offset for
> 	TARGET_THUMB1_ONLY.
>
>
> ARM-fix-mi-thunks-TARGET_THUMB1_ONLY-rev2.patch
>
>
> Index: gcc/config/arm/arm.c
> ===================================================================
> --- gcc/config/arm/arm.c	(revision 199695)
> +++ gcc/config/arm/arm.c	(working copy)
> @@ -25217,7 +25217,12 @@
>   	{
>   	  /* Output ".word .LTHUNKn-7-.LTHUNKPCn".  */
>   	  rtx tem = XEXP (DECL_RTL (function), 0);
> -	  tem = gen_rtx_PLUS (GET_MODE (tem), tem, GEN_INT (-7));
> +	  /* When supported, thunks are generated in ARM mode.  But for
> +	     TARGET_THUMB1_ONLY the thunk is in Thumb mode, so the PC
> +	     pipeline offset is four rather than eight.  Adjust the
> +	     offset accordingly.  */
> +	  tem = gen_rtx_PLUS (GET_MODE (tem), tem,
> +			      GEN_INT (TARGET_THUMB1_ONLY ? -3 : -7));
>   	  tem = gen_rtx_MINUS (GET_MODE (tem),
>   			       tem,
>   			       gen_rtx_SYMBOL_REF (Pmode,
>
Cesar Philippidis June 10, 2013, 4:02 p.m. UTC | #2 
On 6/10/13 8:32 AM, Richard Earnshaw wrote:
> On 07/06/13 17:50, Cesar Philippidis wrote:
>> On 6/6/13 9:00 AM, Richard Earnshaw wrote:
>>> The pipeline offset is 4 for Thumb2 as well.  So at the very least you
>>> need to explain why your change doesn't apply then as well.
>>
>> Yes some context is lost in that comment.  Thunks are usually emitted in
>> ARM mode, except for Thumb-only targets.  Is the new comment OK?  If so,
>> please check it in since I do not have SVN write access.
>>
> 
> So what about ARMv7-M, which is thumb2 but no ARM state?

Thumb2 is handled differently. This patch is inside an if
(TARGET_THUMB1) block.

Cesar
Julian Brown June 11, 2013, 2:42 p.m. UTC | #3 
On Mon, 10 Jun 2013 09:02:24 -0700
Cesar Philippidis <cesar@codesourcery.com> wrote:

> On 6/10/13 8:32 AM, Richard Earnshaw wrote:
> > On 07/06/13 17:50, Cesar Philippidis wrote:
> >> On 6/6/13 9:00 AM, Richard Earnshaw wrote:
> >>> The pipeline offset is 4 for Thumb2 as well.  So at the very
> >>> least you need to explain why your change doesn't apply then as
> >>> well.
> >>
> >> Yes some context is lost in that comment.  Thunks are usually
> >> emitted in ARM mode, except for Thumb-only targets.  Is the new
> >> comment OK?  If so, please check it in since I do not have SVN
> >> write access.
> >>
> > 
> > So what about ARMv7-M, which is thumb2 but no ARM state?
> 
> Thumb2 is handled differently. This patch is inside an if
> (TARGET_THUMB1) block.

To expand, with examples (from g++.old-deja/g++.jason/thunk2.C):

-O2 -S -march=armv4t -fPIC -mthumb:

        .code 32
        .type   _ZThn4_N8CExample9MixinFuncEi1A, %function
_ZThn4_N8CExample9MixinFuncEi1A:
        .fnstart
.LFB19:
        ldr     r12, .LTHUMBFUNC0
.LTHUNKPC0:
        add     r12, pc, r12
        sub     r1, r1, #4
        bx      r12
        .align  2
.LTHUMBFUNC0:
        .word   .LTHUNK0-(.LTHUNKPC0+7)

(Thumb-1 mode, but thunk is in ARM mode, PC offset is 8.)

-O2 -S -march=armv6-m -fPIC -mthumb:

        .code   16
        .thumb_func
        .type   _ZThn4_N8CExample9MixinFuncEi1A, %function
_ZThn4_N8CExample9MixinFuncEi1A:
        .fnstart
.LFB19:
        push {r3}
        ldr     r3, .LTHUMBFUNC0
.LTHUNKPC0:
        add     r3, pc, r3
        mov r12, r3
        sub     r1, r1, #4
        pop     {r3}
        bx      r12
        .align  2
.LTHUMBFUNC0:
        .word   .LTHUNK0-(.LTHUNKPC0+7)

(Current *wrong* behaviour: Thumb mode thunk, but the pipeline offset is
still 8.)

-O2 -S -march=armv7-m -fPIC -mthumb:

        .thumb
        .thumb_func
        .type   _ZThn4_N8CExample9MixinFuncEi1A, %function
_ZThn4_N8CExample9MixinFuncEi1A:
        .fnstart
.LFB19:
        @ args = 0, pretend = 0, frame = 0
        @ frame_needed = 0, uses_anonymous_args = 0
        sub     r1, r1, #4
        b       .LTHUNK0(PLT)

(IIUC, wide branches are available, so there's no need for a synthesised
long PC-relative branch[-exchange]. Similar code is generated for ARM
mode on other architecture versions.)

Ahh -- so actually, maybe the right thing to do is use this last style
of thunk for v6-m, the same as v7-m -- is there any reason that
wouldn't work? (I wrote this patch to start with, but I don't remember
if I considered that possibility at the time...)

HTH,

Julian
Julian Brown June 11, 2013, 3:03 p.m. UTC | #4 
On Tue, 11 Jun 2013 15:42:24 +0100
Julian Brown <julian@codesourcery.com> wrote:

> Ahh -- so actually, maybe the right thing to do is use this last style
> of thunk for v6-m, the same as v7-m -- is there any reason that
> wouldn't work? (I wrote this patch to start with, but I don't remember
> if I considered that possibility at the time...)

Scrub that -- I mistakenly thought that v6m had long branch
instructions. It doesn't (only long BL), so that won't work.

Julian
Cesar Philippidis June 20, 2013, 2:25 p.m. UTC | #5 
Ping.

Cesar


On 6/7/13 9:50 AM, Cesar Philippidis wrote:
> On 6/6/13 9:00 AM, Richard Earnshaw wrote:
>> The pipeline offset is 4 for Thumb2 as well.  So at the very least you
>> need to explain why your change doesn't apply then as well.
> 
> Yes some context is lost in that comment.  Thunks are usually emitted in
> ARM mode, except for Thumb-only targets.  Is the new comment OK?  If so,
> please check it in since I do not have SVN write access.
> 
> Thanks,
> Cesar
> 
> 
> 2013-06-07  Julian Brown  <julian@codesourcery.com>
> 	    Cesar Philippidis  <cesar@codesourcery.com>
> 
> 	gcc/
> 	* config/arm/arm.c (arm_output_mi_thunk): Fix offset for
> 	TARGET_THUMB1_ONLY.
>
diff mbox

Patch

Index: gcc/config/arm/arm.c
===================================================================
--- gcc/config/arm/arm.c	(revision 199695)
+++ gcc/config/arm/arm.c	(working copy)
@@ -25217,7 +25217,12 @@ 
 	{
 	  /* Output ".word .LTHUNKn-7-.LTHUNKPCn".  */
 	  rtx tem = XEXP (DECL_RTL (function), 0);
-	  tem = gen_rtx_PLUS (GET_MODE (tem), tem, GEN_INT (-7));
+	  /* When supported, thunks are generated in ARM mode.  But for 
+	     TARGET_THUMB1_ONLY the thunk is in Thumb mode, so the PC 
+	     pipeline offset is four rather than eight.  Adjust the 
+	     offset accordingly.  */
+	  tem = gen_rtx_PLUS (GET_MODE (tem), tem,
+			      GEN_INT (TARGET_THUMB1_ONLY ? -3 : -7));
 	  tem = gen_rtx_MINUS (GET_MODE (tem),
 			       tem,
 			       gen_rtx_SYMBOL_REF (Pmode,