[Bug target/49799] New: gcc arm generates illegal sbfx instruction

[Bug target/49799] New: gcc arm generates illegal sbfx instruction

[carrot at google dot com]

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

           Summary: gcc arm generates illegal sbfx instruction
           Product: gcc
           Version: 4.7.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: carrot@google.com
              Host: linux
            Target: armeabi


Compile the following code with options -march=armv7-a -O2

extern __inline int bar(int a)
{
    int tmp;

    if (a <= 0) a ^= 0xFFFFFFFF;

    return tmp - 1;
}

void foo(short *K)
{
    short tmp;
    short *pptr, P[14];

    pptr = P;
    tmp = bar(*K);
    *pptr = (*K << tmp) >> 16;

    if (*P < tmp)
        *K++ = 0;
}

gcc 4.7 generates

foo:
    ldrsh    r3, [r0, #0]
    sbfx    r3, r3, #17, #16    // A
    sxth    r3, r3
    cmn    r3, #1
    movlt    r3, #0
    strlth    r3, [r0, #0]    @ movhi
    bx    lr

The sbfx instruction is illegal since the bit position + width > 32.

Although the source code is ill formed (function bar returns undefined value),
compiler should never generate illegal instructions.

gcc4.6 has the same problem.

[Bug target/49799] gcc arm generates illegal sbfx instruction

[mikpe at it dot uu.se]

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

Mikael Pettersson <mikpe at it dot uu.se> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |mikpe at it dot uu.se

--- Comment #1 from Mikael Pettersson <mikpe at it dot uu.se> 2011-07-21 11:10:45 UTC ---
gcc-4.5.3 and gcc-4.4.6 also generate the invalid sbfx, gcc-4.3.6 appears to
work and generates

foo:
        ldrsh   r3, [r0, #0]
        mvn     r2, #0
        mov     r3, r3, asl r2
        cmp     r2, r3, asr #16
        movgt   r3, #0  @ movhi
        strgth  r3, [r0, #0]    @ movhi
        bx      lr

[Bug target/49799] gcc arm generates illegal sbfx instruction

[mikpe at it dot uu.se]

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

Mikael Pettersson <mikpe at it dot uu.se> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |rearnsha at gcc dot gnu.org

--- Comment #2 from Mikael Pettersson <mikpe at it dot uu.se> 2011-07-21 12:53:19 UTC ---
It's triggered by:

Author: rearnsha
Date: Tue Jan 13 14:09:50 2009
New Revision: 143338

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=143338
Log:
    * arm.c (struct processors): Pass for speed down into cost helper
    functions.
    (const_ok_for_op): Handle COMPARE and inequality nodes.
    (arm_rtx_costs_1): Rewrite.
    (arm_size_rtx_costs): Update prototype.
    (arm_rtx_costs): Pass speed down to helper functions.
    (arm_slowmul_rtx_costs): Rework cost calculations.
    (arm_fastmul_rtx_costs, arm_xscale_rtx_costs): Likewise.
    (arm_9e_rtx_costs): Likewise.

It looks like the cost tweaks exposed a pre-existing bug.

[Bug target/49799] gcc arm generates illegal sbfx instruction

[ramana at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2011.07.21 14:27:59
                 CC|                            |ramana at gcc dot gnu.org
     Ever Confirmed|0                           |1

--- Comment #3 from Ramana Radhakrishnan <ramana at gcc dot gnu.org> 2011-07-21 14:27:59 UTC ---
Confirmed - and it appears as though combine is doing this transformation .
Based on a quick debug I couldn't spot any pattern in the backend that
encourages this other than the extv matcher. 


One could guard this by making sure that the backend doesn't recognize such
forms but combine really shouldn't be generating such a extv instruction
reading more bits than that are already available.  

Ramana

[Bug rtl-optimization/49799] gcc arm generates illegal sbfx instruction

[carrot at google dot com]

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

Carrot <carrot at google dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
          Component|target                      |rtl-optimization

--- Comment #4 from Carrot <carrot at google dot com> 2011-07-25 08:39:27 UTC ---
After some combine, gcc gets the following expression

(ashiftrt:SI (ashift:SI (reg:SI 145 [ *K_2(D) ])
        (const_int -1 [0xffffffffffffffff]))
    (const_int 16 [0x10]))

Then gcc tries to simplify it by calling make_compound_operation, in this
function at line 7786, it detects the cases that shift left and right and
replaces it with bit field extraction. It is a good optimization for usual
cases. But for this ill formed case, we can't make a valid bit field extraction
since we don't know what's the target field.

Add the following condition for the transformation should prevent the illegal
instruction.
  INTVAL (XEXP (lhs, 1)) >= 0

[Bug rtl-optimization/49799] gcc arm generates illegal sbfx instruction

[rearnsha at gcc dot gnu.org]

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

--- Comment #5 from Richard Earnshaw <rearnsha at gcc dot gnu.org> 2011-07-25 09:03:30 UTC ---
We should never generate a shift of -1.  Instead the code that does that should
return (clobber const_int 0).

[Bug rtl-optimization/49799] gcc arm generates illegal sbfx instruction

[carrot at google dot com]

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

--- Comment #6 from Carrot <carrot at google dot com> 2011-07-25 09:25:22 UTC ---
(In reply to comment #5)
> We should never generate a shift of -1.  Instead the code that does that should
> return (clobber const_int 0).

I'm afraid this method may impact gcc too much. I added the following to the
source code

K[1] = K[1] << -1;

gcc only generates a warning

auto_corr_to_refl_coef.i:19:5: warning: left shift count is negative [enabled
by default]

and generates following code

ldrsh   r1, [r0, #2]
mvn     ip, #0
mov     r1, r1, asl ip
strh    r1, [r0, #2]

What do you think?

[Bug rtl-optimization/49799] gcc arm generates illegal sbfx instruction

[rearnsha at gcc dot gnu.org]

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

--- Comment #7 from Richard Earnshaw <rearnsha at gcc dot gnu.org> 2011-07-25 09:45:51 UTC ---
No, you miss the point.

Internally we must not generate (ashift (reg) (const_int)) where the const is
negative.

Note that your testcasegenerates a reg shift.

[Bug rtl-optimization/49799] gcc arm generates illegal sbfx instruction

[carrot at google dot com]

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

--- Comment #8 from Carrot <carrot at google dot com> 2011-07-26 02:51:39 UTC ---
(In reply to comment #7)
> No, you miss the point.
> 
> Internally we must not generate (ashift (reg) (const_int)) where the const is
> negative.
> 
> Note that your testcasegenerates a reg shift.

There is no (ashift (reg) (const_int -1)) generated before combine. After
expand, I got:

(insn 6 5 7 3 (set (reg:SI 144)
        (plus:SI (reg/v:SI 141 [ tmp ])
            (const_int -1 [0xffffffffffffffff]))) auto_corr_to_refl_coef.i:7 -1
     (nil))

(insn 7 6 8 3 (set (reg/v:SI 134 [ tmp ])
        (zero_extend:SI (subreg:HI (reg:SI 144) 0)))
auto_corr_to_refl_coef.i:16 -1
     (nil))

(insn 8 7 9 3 (set (reg:SI 145)
        (sign_extend:SI (mem:HI (reg/v/f:SI 143 [ K ]) [2 *K_2(D)+0 S2 A16])))
auto_corr_to_refl_coef.i:17 -1
     (nil))

(insn 9 8 10 3 (set (reg:SI 146)
        (sign_extend:SI (subreg/s/u:HI (reg/v:SI 134 [ tmp ]) 0)))
auto_corr_to_refl_coef.i:17 -1
     (nil))

(insn 10 9 11 3 (set (reg:SI 147)
        (ashift:SI (reg:SI 145)
            (reg:SI 146))) auto_corr_to_refl_coef.i:17 -1
     (nil))

(insn 11 10 12 3 (set (reg:SI 148)
        (ashiftrt:SI (reg:SI 147)
            (const_int 16 [0x10]))) auto_corr_to_refl_coef.i:17 -1
     (nil))


Note that r141 is an uninitialized reg. So after init-regs pass following insn
is prepended to the previous insn sequence

(insn 45 3 6 2 (set (reg/v:SI 141 [ tmp ])
        (const_int 0 [0])) auto_corr_to_refl_coef.i:7 -1
     (nil))


Then in combine pass the whole sequence is combined to

(insn 8 7 9 2 (set (reg:SI 145 [ *K_2(D) ])
        (sign_extend:SI (mem:HI (reg/v/f:SI 143 [ K ]) [2 *K_2(D)+0 S2 A16])))
auto_corr_to_refl_coef.i:17 166 {*arm_extendhisi2_v6}
     (nil))

(note 9 8 10 2 NOTE_INSN_DELETED)

(note 10 9 11 2 NOTE_INSN_DELETED)

(insn 11 10 13 2 (set (reg:SI 148)
        (sign_extract:SI (reg:SI 145 [ *K_2(D) ])
            (const_int 16 [0x10])
            (const_int 17 [0x11]))) auto_corr_to_refl_coef.i:17 131 {extv}
     (expr_list:REG_DEAD (reg:SI 145 [ *K_2(D) ])
        (nil)))

[Bug rtl-optimization/49799] gcc arm generates illegal sbfx instruction

[carrot at gcc dot gnu.org]

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

--- Comment #9 from carrot at gcc dot gnu.org 2011-07-29 01:27:32 UTC ---
Author: carrot
Date: Fri Jul 29 01:27:29 2011
New Revision: 176911

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=176911
Log:
    PR rtl-optimization/49799

    * combine.c (make_compound_operation): Check if the bit field is valid
    before change it to bit field extraction.

    * gcc.dg/pr49799.c: New test case.


Added:
    trunk/gcc/testsuite/gcc.dg/pr49799.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/combine.c
    trunk/gcc/testsuite/ChangeLog

[Bug rtl-optimization/49799] gcc arm generates illegal sbfx instruction

[carrot at gcc dot gnu.org]

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

--- Comment #10 from carrot at gcc dot gnu.org 2011-07-29 08:36:11 UTC ---
Author: carrot
Date: Fri Jul 29 08:35:59 2011
New Revision: 176917

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=176917
Log:
    PR rtl-optimization/49799

    * combine.c (make_compound_operation): Check if the bit field is valid
    before change it to bit field extraction.

    * gcc.dg/pr49799.c: New test case.


Added:
    branches/gcc-4_6-branch/gcc/testsuite/gcc.dg/pr49799.c
      - copied unchanged from r176911, trunk/gcc/testsuite/gcc.dg/pr49799.c
Modified:
    branches/gcc-4_6-branch/   (props changed)
    branches/gcc-4_6-branch/gcc/ChangeLog
    branches/gcc-4_6-branch/gcc/combine.c
    branches/gcc-4_6-branch/gcc/testsuite/ChangeLog

Propchange: branches/gcc-4_6-branch/
            ('svn:mergeinfo' modified)

[Bug rtl-optimization/49799] gcc arm generates illegal sbfx instruction

[carrot at google dot com]

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

Carrot <carrot at google dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|                            |FIXED

--- Comment #11 from Carrot <carrot at google dot com> 2011-08-02 09:36:20 UTC ---
fixed by r176911.

[Bug rtl-optimization/49799] gcc arm generates illegal sbfx instruction

[ramana at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=49799

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |david.gilbert at linaro dot org

--- Comment #12 from Ramana Radhakrishnan <ramana at gcc dot gnu.org> ---
*** Bug 48803 has been marked as a duplicate of this bug. ***