[Bug target/50106] New: [ARM] Wrong code with -march=armv5t -mthumb -Os

[Bug target/50106] New: [ARM] Wrong code with -march=armv5t -mthumb -Os

[sebastian.huber@embedded-brains.de]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50106

             Bug #: 50106
           Summary: [ARM] Wrong code with -march=armv5t -mthumb -Os
    Classification: Unclassified
           Product: gcc
           Version: 4.6.1
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: sebastian.huber@embedded-brains.de
            Target: arm-rtemseabi4.11


Created attachment 25028
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=25028
Sample code.

Command line:

arm-rtemseabi4.11-g++ -march=armv5t -mthumb -Os -S compiler1.test.ii -o
compiler1.test.eabi.Os.s
arm-rtemseabi4.11-g++ -march=armv5t -mthumb -O2 -S compiler1.test.ii -o
compiler1.test.eabi.O2.s

Relevant function:

extern _TestStruct _GetIDS(unsigned int Index)
{
  if (Index < _LIST_SIZE) {
    return _List[Index];
  } else {
    return _List[0];
  }
}

Partial content of compiler1.test.eabi.Os.s with comments:

        .text
        .align  1
        .global _Z7_GetIDSj
        .code   16
        .thumb_func
        .type   _Z7_GetIDSj, %function
_Z7_GetIDSj:
        .fnstart
.LFB0:
        .save   {r0, r1, r2, lr}
        push    {r0, r1, r2, lr}

Why do we save the volatile registers r0, r1, r2 here?

        ldr     r3, .L4
        ldr     r1, .L4+4
        ldr     r3, [r3]
        cmp     r0, r3
        bcs     .L2
        lsl     r0, r0, #1
        add     r1, r1, r0
.L2:
        mov     r2, #2
        add     r0, sp, #4
        bl      memcpy

Here we copy two bytes from the table into the stack.

        add     r3, sp, #4
        ldrb    r0, [r3, #1]
        ldrb    r2, [r3]

Here we copy two bytes from the stack into two registers.

        lsl     r0, r0, #8
        orr     r0, r2

Here we combine the two registers into one.

        @ sp needed for prologue
        pop     {r0, r1, r2, pc}

Here we overwrite our result (r0) with the value of r0 at the function entry.

.L5:
        .align  2
.L4:
        .word   .LANCHOR0
        .word   .LANCHOR1

With -O2 the problem vanishes (because we don't use memcpy()).

[Bug target/50106] [ARM] Wrong code with -march=armv5t -mthumb -Os

[sebastian.huber@embedded-brains.de]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50106

--- Comment #1 from Sebastian Huber <sebastian.huber@embedded-brains.de> 2011-08-17 08:53:00 UTC ---
Created attachment 25029
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=25029
arm-rtemseabi4.11-g++ -march=armv5t -mthumb -Os -S compiler1.test.ii -o
compiler1.test.eabi.Os.s

[Bug target/50106] [ARM] Wrong code with -march=armv5t -mthumb -Os

[sebastian.huber@embedded-brains.de]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50106

--- Comment #2 from Sebastian Huber <sebastian.huber@embedded-brains.de> 2011-08-17 08:54:55 UTC ---
Created attachment 25030
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=25030
arm-rtemseabi4.11-g++ -march=armv5t -mthumb -O2 -S compiler1.test.ii -o
compiler1.test.eabi.O2.s

[Bug target/50106] [ARM] Wrong code with -march=armv5t -mthumb -Os

[ramana at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50106

Ramana Radhakrishnan <ramana at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |wrong-code
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2011-08-17
                 CC|                            |ramana at gcc dot gnu.org
     Ever Confirmed|0                           |1
      Known to fail|                            |4.6.1, 4.7.0

--- Comment #3 from Ramana Radhakrishnan <ramana at gcc dot gnu.org> 2011-08-17 17:19:14 UTC ---
Looks suspiciously similar to PR45070.

Ramana

The patch below *might* fix it - completely untested.

diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
index fc2fd47..affa7b3 100644
--- a/gcc/config/arm/arm.c
+++ b/gcc/config/arm/arm.c
@@ -21124,7 +21124,7 @@ thumb_unexpanded_epilogue (void)
   if (extra_pop > 0)
     {
       unsigned long extra_mask = (1 << extra_pop) - 1;
-      live_regs_mask |= extra_mask << (size / UNITS_PER_WORD);
+      live_regs_mask |= extra_mask << ((size + 3) / UNITS_PER_WORD);
     }

   /* The prolog may have pushed some high registers to use as

[Bug target/50106] [ARM] Wrong code with -march=armv5t -mthumb -Os

[sebastian.huber@embedded-brains.de]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50106

--- Comment #4 from Sebastian Huber <sebastian.huber@embedded-brains.de> 2011-08-22 09:43:39 UTC ---
Yes, this patch fixes the problem.

It is still not clear to me why we save the volatile registers r0, r1, and r2
at all.  Also we restore r1, r2, and r3.  Does this make sense?  I think also
the the usage of memcpy() is not justified in this case.

_Z7_GetIDSj:
        .fnstart
.LFB0:
        .save   {r0, r1, r2, lr}
        push    {r0, r1, r2, lr}
        ldr     r1, .L4
        cmp     r0, #3
        bhi     .L2
        lsl     r0, r0, #1
        add     r1, r1, r0
.L2:
        mov     r2, #2
        add     r0, sp, #4
        bl      memcpy
        add     r3, sp, #4
        ldrb    r0, [r3, #1]
        ldrb    r2, [r3]
        lsl     r0, r0, #8
        orr     r0, r2
        @ sp needed for prologue
        pop     {r1, r2, r3, pc}

[Bug target/50106] [ARM] Wrong code with -march=armv5t -mthumb -Os

[sebastian.huber@embedded-brains.de]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50106

Sebastian Huber <sebastian.huber@embedded-brains.de> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Target|arm-rtemseabi4.11           |arm-eabi-gcc

--- Comment #5 from Sebastian Huber <sebastian.huber@embedded-brains.de> 2011-09-12 09:23:29 UTC ---
It would be nice if this can be fixed for 4.6.2 and 4.7.0.

[Bug target/50106] [ARM] Wrong code with -march=armv5t -mthumb -Os

[sebastian.huber@embedded-brains.de]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50106

--- Comment #6 from Sebastian Huber <sebastian.huber@embedded-brains.de> 2011-10-18 14:19:55 UTC ---
Created attachment 25543
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=25543
arm-eabi-g++ -march=armv5t -mthumb -Os -S compiler1.test.ii -o
compiler1.test.eabi.GCC-4.5.4.Os.s

[Bug target/50106] [ARM] Wrong code with -march=armv5t -mthumb -Os

[ramana at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50106

Ramana Radhakrishnan <ramana at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
         AssignedTo|unassigned at gcc dot       |ramana at gcc dot gnu.org
                   |gnu.org                     |

--- Comment #7 from Ramana Radhakrishnan <ramana at gcc dot gnu.org> 2011-10-18 14:58:13 UTC ---
I finished testing this with some other patches and backports that I had.
Should commit in the next day or two. 

Ramana

[Bug target/50106] [ARM] Wrong code with -march=armv5t -mthumb -Os

[ramana at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50106

--- Comment #8 from Ramana Radhakrishnan <ramana at gcc dot gnu.org> 2011-10-20 09:07:36 UTC ---
Author: ramana
Date: Thu Oct 20 09:07:30 2011
New Revision: 180240

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=180240
Log:


2011-10-20  Ramana Radhakrishnan  <ramana.radhakrishnan@linaro.org>

       PR target/50106
       * config/arm/arm.c (thumb_unexpanded_epilogue): Handle return
    reg size from 1-3.


Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/config/arm/arm.c

[Bug target/50106] [ARM] Wrong code with -march=armv5t -mthumb -Os

[ramana at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50106

--- Comment #9 from Ramana Radhakrishnan <ramana at gcc dot gnu.org> 2011-10-20 09:24:10 UTC ---
Author: ramana
Date: Thu Oct 20 09:24:06 2011
New Revision: 180241

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=180241
Log:

Backport from mainline fix for PR target/50106.

Modified:
    branches/gcc-4_6-branch/gcc/ChangeLog
    branches/gcc-4_6-branch/gcc/config/arm/arm.c

[Bug target/50106] [ARM] Wrong code with -march=armv5t -mthumb -Os

[ramana at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50106

Ramana Radhakrishnan <ramana at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|                            |FIXED
   Target Milestone|---                         |4.6.2

--- Comment #10 from Ramana Radhakrishnan <ramana at gcc dot gnu.org> 2011-10-20 09:25:10 UTC ---
Fixed now I think.

[Bug target/50106] [ARM] Wrong code with -march=armv5t -mthumb -Os

[sebastian.huber@embedded-brains.de]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50106

--- Comment #11 from Sebastian Huber <sebastian.huber@embedded-brains.de> 2011-10-20 11:07:09 UTC ---
Thank you very much.  With this change the GCC 4.6.2-RC-20111019 produces now
correct code in this case.

I know understand why the unused volatile registers are saved and restored. 
This is to get rid of the arithmetic stack adjustments.

--- test.Os.GCC-4.5.s   2011-10-20 13:04:15.384638860 +0200
+++ test.Os.GCC-4.6.s   2011-10-20 13:04:15.396639237 +0200
@@ -17,32 +17,29 @@
        .thumb_func
        .type   _GetIDS, %function
 _GetIDS:
-       push    {lr}
-       ldr     r2, .L4
-       sub     sp, sp, #12
-       ldr     r2, [r2]
-       mov     r3, r0
+       push    {r0, r1, r2, lr}
+       ldr     r3, .L4
        ldr     r1, .L4+4
-       add     r0, sp, #4
-       cmp     r3, r2
+       ldr     r3, [r3]
+       cmp     r0, r3
        bge     .L2
-       lsl     r3, r3, #1
-       add     r1, r1, r3
+       lsl     r0, r0, #1
+       add     r1, r1, r0
 .L2:
        mov     r2, #2
+       add     r0, sp, #4
        bl      memcpy
        add     r3, sp, #4
        ldrb    r0, [r3, #1]
        ldrb    r2, [r3]
        lsl     r0, r0, #8
-       add     sp, sp, #12
-       orr     r0, r0, r2
+       orr     r0, r2
        @ sp needed for prologue
-       pop     {pc}
+       pop     {r1, r2, r3, pc}
 .L5:
        .align  2
 .L4:
        .word   _LIST_SIZE
        .word   _List
        .size   _GetIDS, .-_GetIDS
-       .ident  "GCC: (GNU) 4.5.4 20111013 (prerelease)"
+       .ident  "GCC: (GNU) 4.6.2 20111019 (prerelease)"

[Bug target/50106] [ARM] Wrong code with -march=armv5t -mthumb -Os

[amker at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50106

--- Comment #12 from amker at gcc dot gnu.org 2012-06-12 02:50:37 UTC ---
Author: amker
Date: Tue Jun 12 02:50:34 2012
New Revision: 188416

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=188416
Log:
    Backport r180240 from mainline
    2011-10-20  Ramana Radhakrishnan  <ramana.radhakrishnan@linaro.org>

    PR target/50106
    * config/arm/arm.c (thumb_unexpanded_epilogue): Handle return
    reg size from 1-3.

Modified:
    branches/ARM/embedded-4_6-branch/gcc/ChangeLog.arm
    branches/ARM/embedded-4_6-branch/gcc/config/arm/arm.c