[Bug rtl-optimization/52060] New: Incorrect mask/and (ARM "bic") instruction generated for shifted expression parameter, triggered by -O2 -finline-functions

[Bug rtl-optimization/52060] New: Incorrect mask/and (ARM "bic") instruction generated for shifted expression parameter, triggered by -O2 -finline-functions

[swarren at nvidia dot com]

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

             Bug #: 52060
           Summary: Incorrect mask/and (ARM "bic") instruction generated
                    for shifted expression parameter, triggered by -O2
                    -finline-functions
    Classification: Unclassified
           Product: gcc
           Version: 4.6.2
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: rtl-optimization
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: swarren@nvidia.com


Created attachment 26519
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=26519
Test case source

Compiling the attached test case for ARM with inlining enabled produces a buggy
executable.

(on x86-64 host)

    ${CROSS_COMPILE}gcc -O2 -o testprog-good testprog.c

(on ARM target)

    $ ./testprog-good
    clearValue: 1.000000 -> 65535
    Expected: 65535

(on x86-64 host)

    ${CROSS_COMPILE}gcc -O2 -finline-functions -o testprog-bad testprog.c

(on ARM target)

    $ ./testprog-bad
    clearValue: 1.000000 -> 57344
    Expected: 65535

gcc is gcc-4.6.1 compiled using crosstool-ng (obtained from hg on 2011/06/08),
built to target ARM Cortex-A9 by default. I can supply more details if needed,
but I don't think crosstool-ng is patching/munging/... gcc when building it.

(I also confirmed the bug is still present in gcc-4.6.2)

In both cases, quick_float_to_int() is compiled identically:

000083dc <quick_float_to_int>:
83dc: e1a03400 lsl r3, r0, #8
83e0: e7e72bd0 ubfx r2, r0, #23, #8
83e4: e352007d cmp r2, #125 ; 0x7d
83e8: c3833102 orrgt r3, r3, #-2147483648 ; 0x80000000
83ec: e262209d rsb r2, r2, #157 ; 0x9d
83f0: d3833102 orrle r3, r3, #-2147483648 ; 0x80000000
83f4: c1a01233 lsrgt r1, r3, r2
83f8: d1a01233 lsrle r1, r3, r2
83fc: e3e00000 mvn r0, #0
8400: e1c33210 bic r3, r3, r0, lsl r2
8404: c281c001 addgt ip, r1, #1
8408: e2011002 and r1, r1, #2
840c: c1a0c0ac lsrgt ip, ip, #1
8410: d3a0c000 movle ip, #0
8414: e1931001 orrs r1, r3, r1
8418: 03e00001 mvneq r0, #1
841c: e000000c and r0, r0, ip
8420: e12fff1e bx lr

When inlining is not enabled, convert() calls quick_float_to_int():

(good)
00008424 <convert>:
8424: e30f1ff0 movw r1, #65520 ; 0xfff0
8428: e92d4008 push {r3, lr}
842c: e344197f movt r1, #18815 ; 0x497f
8430: eb00001a bl 84a0 <__aeabi_fmul>
8434: ebffffe8 bl 83dc <quick_float_to_int>
8438: e3083808 movw r3, #34824 ; 0x8808
843c: e1a026a0 lsr r2, r0, #13
8440: e3403000 movt r3, #0
8444: e7933102 ldr r3, [r3, r2, lsl #2]
8448: e2632007 rsb r2, r3, #7
844c: e1a00230 lsr r0, r0, r2
8450: e1a00980 lsl r0, r0, #19
8454: e1a009a0 lsr r0, r0, #19
8458: e1800683 orr r0, r0, r3, lsl #13
845c: e8bd8008 pop {r3, pc}

When inlining is enabled, convert() inlines quick_float_to_int():

(bad)
00008424 <convert>:
8424: e30f1ff0 movw r1, #65520 ; 0xfff0
8428: e92d4008 push {r3, lr}
842c: e344197f movt r1, #18815 ; 0x497f
8430: eb000024 bl 84c8 <__aeabi_fmul>
8434: e7e73bd0 ubfx r3, r0, #23, #8
8438: e59f2044 ldr r2, [pc, #68] ; 8484 <convert+0x60>
843c: e353007d cmp r3, #125 ; 0x7d
8440: c1a00400 lslgt r0, r0, #8
8444: c263309d rsbgt r3, r3, #157 ; 0x9d
8448: d3a03000 movle r3, #0
844c: c3800102 orrgt r0, r0, #-2147483648 ; 0x80000000
8450: c1a03330 lsrgt r3, r0, r3
8454: c2833001 addgt r3, r3, #1
8458: c1a030a3 lsrgt r3, r3, #1
845c: e1a016a3 lsr r1, r3, #13
8460: e3c33d7f bic r3, r3, #8128 ; 0x1fc0
8464: e3c331fe bic r3, r3, #-2147483585 ; 0x8000003f
8468: e7920101 ldr r0, [r2, r1, lsl #2]
846c: e2602007 rsb r2, r0, #7
8470: e1a03233 lsr r3, r3, r2
8474: e1a03983 lsl r3, r3, #19
8478: e1a039a3 lsr r3, r3, #19
847c: e1830680 orr r0, r3, r0, lsl #13
8480: e8bd8008 pop {r3, pc}
8484: 00008830 .word 0x00008830

Here's the problem:

C variable "linear" is in:
Good code: Register r0 at address 0x843c.
Bad code: Register r3 at address 0x845c.

In both cases, the value is identical (checked with gdb)

This value is then used in the expression:

nl = (lOnes<<13) | ((linear>>(7-lOnes))&0x1fff); //[15:13] leading ones

This is implemented starting at 0x8448 in the good code and 0x846c in the bad
code.

In the good code, "linear" (r0) doesn't change between those two points. In the
bad code, "linear" (r3) does change due to the two "bic" instructions at
address 0x8460. Those two bic instructions bitwise-and "linear" with
0x7fffe000. "linear" is 0xfffff, and this results in linear==0xfe000. I think
the compiler is synthesizing the bic instructions because of the "& 0x1fff" in
the expression, but when applying it to "linear", it's failing to shift the
mask left by "7-lOnes" when applying it to the unshifted value of "linear". In
fact, the compiler doesn't even need to do the mask separately, since that's
what the lsl/lsr #19 at address 0x8474 in the bad code are doing anyway. The
good code doesn't have any such bic instructions.

This issue did not occur using CodeSourcery's 2009q1-203 compiler, which is
gcc-4.3.3.

Note that putting a printf() inside convert() after the call to linear, or
assigning linear to some global variable, will cause the bug not to occur.

[Bug rtl-optimization/52060] Incorrect mask/and (ARM "bic") instruction generated for shifted expression parameter, triggered by -O2 -finline-functions

[swarren at nvidia dot com]

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

--- Comment #1 from Stephen Warren <swarren at nvidia dot com> 2012-01-30 22:47:27 UTC ---
gcc-linaro 2012.01 has the same issue, although the code it generated was a
little different.

gcc-4.7 snapshot 20120128 appears to have the same issue.

(I couldn't build the whole 4.7 compiler due to configure issues with
GCC_NO_EXECUTABLES in the final phase, and hence couldn't link an application,
but the cut-down gcc executables from the earlier build phases could compile to
object files just not link. The disassembly of the .o files generated by those
early-phase gccs indicate the same issue)

[Bug target/52060] Incorrect mask/and (ARM "bic") instruction generated for shifted expression parameter, triggered by -O2 -finline-functions

[swarren at nvidia dot com]

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

--- Comment #2 from Stephen Warren <swarren at nvidia dot com> 2012-01-30 23:16:11 UTC ---
gcc-4.5.3 appears unaffected.

[Bug target/52060] Incorrect mask/and (ARM "bic") instruction generated for shifted expression parameter, triggered by -O2 -finline-functions

[ibolton at gcc dot gnu.org]

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

Ian Bolton <ibolton at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2012-02-03
                 CC|                            |ibolton at gcc dot gnu.org
     Ever Confirmed|0                           |1
      Known to fail|                            |4.7.0

--- Comment #3 from Ian Bolton <ibolton at gcc dot gnu.org> 2012-02-03 16:49:55 UTC ---
Confirmed on trunk (r183840).

[Bug target/52060] Incorrect mask/and (ARM "bic") instruction generated for shifted expression parameter, triggered by -O2 -finline-functions

[ramana at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |ramana at gcc dot gnu.org
      Known to work|                            |4.5.3

--- Comment #4 from Ramana Radhakrishnan <ramana at gcc dot gnu.org> 2012-02-03 18:49:24 UTC ---
According to comment #2 appears to work with 4.5.3 which means we need to look
at this carefully.

[Bug rtl-optimization/52060] Incorrect mask/and (ARM "bic") instruction generated for shifted expression parameter, triggered by -O2 -finline-functions

[rearnsha at gcc dot gnu.org]

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

Richard Earnshaw <rearnsha at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Last reconfirmed|2012-02-03 00:00:00         |2012-02-06 0:00
          Component|target                      |rtl-optimization

--- Comment #5 from Richard Earnshaw <rearnsha at gcc dot gnu.org> 2012-02-06 14:28:20 UTC ---
Looks like a combine bug to me.  We have the following three insns passed to
combine:

i3 = (insn 49 48 50 5 (set (reg:SI 192)
        (lshiftrt:SI (reg/v:SI 155 [ linear ])
            (const_int 13 [0xd]))) bug.c:47 125 {*arm_shiftsi3}
     (nil))

i2 = (insn 46 72 48 5 (set (reg/v:SI 155 [ linear ])
        (and:SI (reg:SI 154 [ iftmp.1 ])
            (reg:SI 176 [ prephitmp.29 ]))) bug.c:24 75 {*arm_andsi3_insn}
     (expr_list:REG_DEAD (reg:SI 176 [ prephitmp.29 ])
        (expr_list:REG_DEAD (reg:SI 154 [ iftmp.1 ])
            (nil))))

i1 = (insn 72 41 46 5 (set (reg:SI 154 [ iftmp.1 ])
        (if_then_else:SI (eq (reg:CC_NOOV 24 cc)
                (const_int 0 [0]))
            (const_int -2 [0xfffffffffffffffe])
            (const_int -1 [0xffffffffffffffff]))) bug.c:23 234 {*movsicc_insn}
     (expr_list:REG_DEAD (reg:CC 24 cc)
        (nil)))

Note that the result of i2 does not die in i3.  Nevertheless, we simplify this
expression to the form

(parallel [
        (set (reg:SI 192)
            (lshiftrt:SI (and:SI (if_then_else:SI (eq (reg:CC_NOOV 24 cc)
                            (const_int 0 [0]))
                        (const_int -8192 [0xffffffffffffe000])
                        (const_int -8192 [0xffffffffffffe000]))
                    (reg:SI 176 [ prephitmp.29 ]))
                (const_int 13 [0xd])))
        (set (reg/v:SI 155 [ linear ])
            (and:SI (if_then_else:SI (eq (reg:CC_NOOV 24 cc)
                        (const_int 0 [0]))
                    (const_int -8192 [0xffffffffffffe000])
                    (const_int -8192 [0xffffffffffffe000]))
                (reg:SI 176 [ prephitmp.29 ])))
    ])

The choice of -8192 for the constant in the if-then-else expressions seems to
come from the fact that i3 will shift the result right by 13 (ie 0x1fff) and
that therefore we can exclude those bits from the and operation (which will
ultimately collapse the if-then-else into a single constant).  However, that is
only true in the first of the two operations in the parallel above.  It's not
true for the second operation.

Over to the combine experts.

[Bug rtl-optimization/52060] [4.7 regression] Invalid constant simplification in combine with parallel result

[jakub at gcc dot gnu.org]

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

--- Comment #6 from Jakub Jelinek <jakub at gcc dot gnu.org> 2012-02-06 17:44:36 UTC ---
Created attachment 26586
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=26586
gcc47-pr52060.patch

I think the important question is if it is ok that combine_simplify_rtx may
modify the given rtl in place, as opposed to keeping it unmodified and just
allocating new rtx and returning it.  A brief look at combine.c suggests that
SUBST () is used in so many places that it is meant to be ok to modify it in
place.

Then we for added_sets_{1,2} should just make copies of i1src resp. i0src
upfront, which means perhaps slightly bigger amount of GC, but at least it
won't miscompile things.

If the modification in place is undesirable, then e.g. force_to_mode is
something that modifies rtx in place very deep into it.

This untested fix cures the testcase on a cross to arm.

[Bug rtl-optimization/52060] [4.7 regression] Invalid constant simplification in combine with parallel result

[ebotcazou at gcc dot gnu.org]

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

--- Comment #7 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2012-02-07 08:46:39 UTC ---
> I think the important question is if it is ok that combine_simplify_rtx may
> modify the given rtl in place, as opposed to keeping it unmodified and just
> allocating new rtx and returning it.  A brief look at combine.c suggests that
> SUBST () is used in so many places that it is meant to be ok to modify it in
> place.
> 
> Then we for added_sets_{1,2} should just make copies of i1src resp. i0src
> upfront, which means perhaps slightly bigger amount of GC, but at least it
> won't miscompile things.

I'm a little uncomfortable with the patch, because I think it's annoying to
have to copy a pattern that you're going to substitute for.  On the other hand,
given that combine_simplify_rtx is allowed to SUBST, that's the most reasonable
fix.

Why do you need 2 copies of I0SRC though?

[Bug rtl-optimization/52060] [4.6/4.7 Regression] Invalid constant simplification in combine with parallel result

[rguenth at gcc dot gnu.org]

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

Richard Guenther <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P3                          |P2
   Target Milestone|---                         |4.6.4
            Summary|[4.7 regression] Invalid    |[4.6/4.7 Regression]
                   |constant simplification in  |Invalid constant
                   |combine with parallel       |simplification in combine
                   |result                      |with parallel result

--- Comment #8 from Richard Guenther <rguenth at gcc dot gnu.org> 2012-02-07 09:18:21 UTC ---
Reported for version 4.6.x so it must be a 4.6/4.7 regression.  And thus P2
only.

[Bug rtl-optimization/52060] [4.6/4.7 Regression] Invalid constant simplification in combine with parallel result

[jakub at gcc dot gnu.org]

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

--- Comment #9 from Jakub Jelinek <jakub at gcc dot gnu.org> 2012-02-07 09:21:34 UTC ---
(In reply to comment #7)
> I'm a little uncomfortable with the patch, because I think it's annoying to
> have to copy a pattern that you're going to substitute for.  On the other hand,
> given that combine_simplify_rtx is allowed to SUBST, that's the most reasonable
> fix.

Yeah, I'm also not very happy about it, but we do other copy_rtx calls (e.g.
for i0pat/i1pat/i2pat), for the same reason.

> Why do you need 2 copies of I0SRC though?

Because it might be substed twice, once into i1pat (which might clobber it with
random changes) and then again into i2pat, if both i1dest and i2dest are needed
after the pattern.  With the two separate variables we copy_rtx it only when we
are actually going to use them in subst.  Other alternative would be to create
just one copy (i0src_copy) upfront
if ((added_sets_1 && i0_feeds_i1_n)
    || (added_sets_2 && ((i0_feeds_i1_n && i1_feeds_i2_n) || (i0_feeds_i2_n))))
and then right before we subst it into i1pat check the second half of the
condition again and if it is true, copy it again, but that would be IMHO uglier
than this.

[Bug rtl-optimization/52060] [4.6/4.7 Regression] Invalid constant simplification in combine with parallel result

[jakub at gcc dot gnu.org]

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

--- Comment #10 from Jakub Jelinek <jakub at gcc dot gnu.org> 2012-02-07 15:48:59 UTC ---
Author: jakub
Date: Tue Feb  7 15:48:52 2012
New Revision: 183972

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=183972
Log:
    PR rtl-optimization/52060
    * combine.c (try_combine): Add i0src_copy and i0src_copy2 variables,
    copy i1src to i1src_copy whenever added_sets_2 && i1_feeds_i2_n already
    before i1dest -> i1src substitution in newpat, copy i0src to i0src_copy
    and/or i0src_copy2 when needed.

    * gcc.dg/torture/pr52060.c: New test.

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

[Bug rtl-optimization/52060] [4.6 Regression] Invalid constant simplification in combine with parallel result

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
         AssignedTo|unassigned at gcc dot       |jakub at gcc dot gnu.org
                   |gnu.org                     |
            Summary|[4.6/4.7 Regression]        |[4.6 Regression] Invalid
                   |Invalid constant            |constant simplification in
                   |simplification in combine   |combine with parallel
                   |with parallel result        |result

--- Comment #11 from Jakub Jelinek <jakub at gcc dot gnu.org> 2012-02-07 16:16:21 UTC ---
Fixed on the trunk so far.

[Bug rtl-optimization/52060] [4.6 Regression] Invalid constant simplification in combine with parallel result

[jakub at gcc dot gnu.org]

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

--- Comment #12 from Jakub Jelinek <jakub at gcc dot gnu.org> 2012-02-09 17:29:52 UTC ---
Author: jakub
Date: Thu Feb  9 17:29:38 2012
New Revision: 184061

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=184061
Log:
    Backported from mainline
    2012-02-07  Jakub Jelinek  <jakub@redhat.com>

    PR rtl-optimization/52060
    * combine.c (try_combine): Add i0src_copy and i0src_copy2 variables,
    copy i1src to i1src_copy whenever added_sets_2 && i1_feeds_i2_n already
    before i1dest -> i1src substitution in newpat, copy i0src to i0src_copy
    and/or i0src_copy2 when needed.

    * gcc.dg/torture/pr52060.c: New test.

Added:
    branches/gcc-4_6-branch/gcc/testsuite/gcc.dg/torture/pr52060.c
Modified:
    branches/gcc-4_6-branch/gcc/ChangeLog
    branches/gcc-4_6-branch/gcc/combine.c
    branches/gcc-4_6-branch/gcc/testsuite/ChangeLog

[Bug rtl-optimization/52060] [4.6 Regression] Invalid constant simplification in combine with parallel result

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

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

--- Comment #13 from Jakub Jelinek <jakub at gcc dot gnu.org> 2012-02-09 17:49:36 UTC ---
Fixed.

[Bug rtl-optimization/52060] [4.6 Regression] Invalid constant simplification in combine with parallel result

[rguenth at gcc dot gnu.org]

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

Richard Guenther <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|4.6.4                       |4.6.3

[Bug rtl-optimization/52060] [4.6 Regression] Invalid constant simplification in combine with parallel result

[jingyu at gcc dot gnu.org]

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

--- Comment #14 from Jing Yu <jingyu at gcc dot gnu.org> 2012-03-02 01:01:31 UTC ---
Author: jingyu
Date: Fri Mar  2 01:01:16 2012
New Revision: 184771

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=184771
Log:
2012-03-01  Jing Yu  <jingyu@google.com>
       Backport r184061 from gcc-4_6-branch to fix PR52060.

       2012-02-07  Jakub Jelinek  <jakub@redhat.com>
       PR rtl-optimization/52060
       * gcc.dg/torture/pr52060.c: New test.

       2012-02-07  Jakub Jelinek  <jakub@redhat.com>
       PR rtl-optimization/52060
       * combine.c (try_combine): Add i0src_copy and i0src_copy2 variables,
       copy i1src to i1src_copy whenever added_sets_2 && i1_feeds_i2_n
       already before i1dest -> i1src substitution in newpat, copy i0src
       to i0src_copy and/or i0src_copy2 when needed.


Added:
    branches/google/gcc-4_6_2-mobile/gcc/testsuite/gcc.dg/torture/pr52060.c
Modified:
    branches/google/gcc-4_6_2-mobile/gcc/ChangeLog.google-4_6
    branches/google/gcc-4_6_2-mobile/gcc/combine.c
    branches/google/gcc-4_6_2-mobile/gcc/testsuite/ChangeLog.google-4_6