Re: [Bug target/65648] New: [5 Regression] Bad code due to IRA fails to recognize the clobber in parallel

Re: [Bug target/65648] New: [5 Regression] Bad code due to IRA fails to recognize the clobber in parallel

[Andrew Pinski]

On Wed, Apr 1, 2015 at 5:44 PM, terry.guo at arm dot com
<gcc-bugzilla@gcc.gnu.org> wrote:
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65648
>
>             Bug ID: 65648
>            Summary: [5 Regression] Bad code due to IRA fails to recognize
>                     the clobber in parallel
>            Product: gcc
>            Version: 5.0
>             Status: UNCONFIRMED
>           Severity: normal
>           Priority: P3
>          Component: target
>           Assignee: unassigned at gcc dot gnu.org
>           Reporter: terry.guo at arm dot com
>
> Created attachment 35200
>   --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=35200&action=edit
> test case
>
> When compile attached case with command:
> arm-none-eabi-gcc -march=armv6-m -mthumb -Os x.c -S
>
> The gcc generates bad code as below:
> main:
>         push    {r0, r1, r2, r4, r5, r6, r7, lr}
>         ldr     r4, .L5
>         movs    r5, #0
>         ldr     r0, [r4, #8]
>         add     r1, sp, #4
>         rsbs    r2, r0, #0
>         adcs    r2, r2, r0
>         subs    r3, r2, r3  //r3 is used but not initialized
>
> The instruction to initialize r3 is removed due to IRA failed to recognize the
> clobber in reload pass. Before the reload pass, we have three instructions like
> below:
> (insn 12 11 14 2 (parallel [
>             (set (reg:SI 128)
>                 (eq:SI (reg:SI 110 [ D.4914 ])
>                     (const_int 0 [0])))
>             (clobber (reg:SI 129))
>         ]) x.c:23 760 {*cstoresi_eq0_thumb1_insn}
>      (expr_list:REG_UNUSED (reg:SI 129)
>         (nil)))
> (insn 20 19 22 2 (set (reg:SI 135)
>         (plus:SI (plus:SI (reg:SI 136)
>                 (reg:SI 137))
>             (geu:SI (reg:SI 131 [ D.4914 ])
>                 (reg:SI 134 [ c ])))) x.c:23 764 {thumb1_addsi3_addgeu}
>      (expr_list:REG_DEAD (reg:SI 137)
>         (expr_list:REG_DEAD (reg:SI 136)
>             (expr_list:REG_DEAD (reg:SI 134 [ c ])
>                 (expr_list:REG_DEAD (reg:SI 131 [ D.4914 ])
>                     (nil))))))
> (insn 22 20 23 2 (set (reg:SI 138)
>         (minus:SI (reg:SI 128)
>             (reg:SI 135))) x.c:23 720 {thumb1_subsi3_insn}
>      (expr_list:REG_DEAD (reg:SI 135)
>         (expr_list:REG_DEAD (reg:SI 128)
>             (nil))))
>
> After the reload pass, those instructions are wrongly turned into:
> (insn 20 53 58 2 (set (reg:SI 3 r3 [135])
>         (plus:SI (plus:SI (reg:SI 3 r3 [137])
>                 (reg:SI 2 r2 [136]))
>             (geu:SI (reg:SI 7 r7 [orig:131 D.4914 ] [131])
>                 (reg:SI 6 r6 [153])))) x.c:23 764 {thumb1_addsi3_addgeu}
>      (nil))
> (insn 58 20 55 2 (parallel [
>             (set (reg:SI 2 r2 [128])
>                 (eq:SI (reg:SI 0 r0 [orig:110 D.4914 ] [110])
>                     (const_int 0 [0])))
>             (clobber (reg:SI 3 r3 [129]))
>         ]) x.c:23 760 {*cstoresi_eq0_thumb1_insn}
>      (nil))
> (note 55 58 22 2 NOTE_INSN_DELETED)
> (insn 22 55 23 2 (set (reg:SI 3 r3 [138])
>         (minus:SI (reg:SI 2 r2 [128])
>             (reg:SI 3 r3 [135]))) x.c:23 720 {thumb1_subsi3_insn}
>      (nil))
>
> However the later pass can recognize the clobber in insn 58 and will remove the
> insn 20 because the r3 defined in insn 20 is clobbered by insn 58.


This is a bug in the arm's thumb1.md file, it uses match_operand when
it should really be using match_scratch.
Looks like most of the arm back-end has this bug in it.

See https://gcc.gnu.org/onlinedocs/gccint/Regs-and-Memory.html#index-scratch-2870
and the corresponding match_scratch documentation.

That is this:

1515 (define_insn "*cstoresi_eq0_thumb1_insn"
1516   [(set (match_operand:SI 0 "s_register_operand" "=&l,l")
1517         (eq:SI (match_operand:SI 1 "s_register_operand" "l,0")
1518                (const_int 0)))
1519    (clobber (match_operand:SI 2 "s_register_operand" "=X,l"))]
1520   "TARGET_THUMB1"
1521   "@
1522    rsbs\\t%0, %1, #0\;adcs\\t%0, %0, %1
1523    rsbs\\t%2, %1, #0\;adcs\\t%0, %1, %2"
1524   [(set_attr "length" "4")
1525    (set_attr "type" "multiple")]
1526 )

Really should be using (clobber (match_scratch:... ))

So it generates a scratch RTL right away and then the register
allocater knows that it is only used in that instruction.

Thanks,
Andrew

[Bug target/65648] [5 Regression] Bad code due to IRA fails to recognize the clobber in parallel

[pinskia at gmail dot com]

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

--- Comment #1 from pinskia at gmail dot com <pinskia at gmail dot com> ---
On Wed, Apr 1, 2015 at 5:44 PM, terry.guo at arm dot com
<gcc-bugzilla@gcc.gnu.org> wrote:
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65648
>
>             Bug ID: 65648
>            Summary: [5 Regression] Bad code due to IRA fails to recognize
>                     the clobber in parallel
>            Product: gcc
>            Version: 5.0
>             Status: UNCONFIRMED
>           Severity: normal
>           Priority: P3
>          Component: target
>           Assignee: unassigned at gcc dot gnu.org
>           Reporter: terry.guo at arm dot com
>
> Created attachment 35200
>   --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=35200&action=edit
> test case
>
> When compile attached case with command:
> arm-none-eabi-gcc -march=armv6-m -mthumb -Os x.c -S
>
> The gcc generates bad code as below:
> main:
>         push    {r0, r1, r2, r4, r5, r6, r7, lr}
>         ldr     r4, .L5
>         movs    r5, #0
>         ldr     r0, [r4, #8]
>         add     r1, sp, #4
>         rsbs    r2, r0, #0
>         adcs    r2, r2, r0
>         subs    r3, r2, r3  //r3 is used but not initialized
>
> The instruction to initialize r3 is removed due to IRA failed to recognize the
> clobber in reload pass. Before the reload pass, we have three instructions like
> below:
> (insn 12 11 14 2 (parallel [
>             (set (reg:SI 128)
>                 (eq:SI (reg:SI 110 [ D.4914 ])
>                     (const_int 0 [0])))
>             (clobber (reg:SI 129))
>         ]) x.c:23 760 {*cstoresi_eq0_thumb1_insn}
>      (expr_list:REG_UNUSED (reg:SI 129)
>         (nil)))
> (insn 20 19 22 2 (set (reg:SI 135)
>         (plus:SI (plus:SI (reg:SI 136)
>                 (reg:SI 137))
>             (geu:SI (reg:SI 131 [ D.4914 ])
>                 (reg:SI 134 [ c ])))) x.c:23 764 {thumb1_addsi3_addgeu}
>      (expr_list:REG_DEAD (reg:SI 137)
>         (expr_list:REG_DEAD (reg:SI 136)
>             (expr_list:REG_DEAD (reg:SI 134 [ c ])
>                 (expr_list:REG_DEAD (reg:SI 131 [ D.4914 ])
>                     (nil))))))
> (insn 22 20 23 2 (set (reg:SI 138)
>         (minus:SI (reg:SI 128)
>             (reg:SI 135))) x.c:23 720 {thumb1_subsi3_insn}
>      (expr_list:REG_DEAD (reg:SI 135)
>         (expr_list:REG_DEAD (reg:SI 128)
>             (nil))))
>
> After the reload pass, those instructions are wrongly turned into:
> (insn 20 53 58 2 (set (reg:SI 3 r3 [135])
>         (plus:SI (plus:SI (reg:SI 3 r3 [137])
>                 (reg:SI 2 r2 [136]))
>             (geu:SI (reg:SI 7 r7 [orig:131 D.4914 ] [131])
>                 (reg:SI 6 r6 [153])))) x.c:23 764 {thumb1_addsi3_addgeu}
>      (nil))
> (insn 58 20 55 2 (parallel [
>             (set (reg:SI 2 r2 [128])
>                 (eq:SI (reg:SI 0 r0 [orig:110 D.4914 ] [110])
>                     (const_int 0 [0])))
>             (clobber (reg:SI 3 r3 [129]))
>         ]) x.c:23 760 {*cstoresi_eq0_thumb1_insn}
>      (nil))
> (note 55 58 22 2 NOTE_INSN_DELETED)
> (insn 22 55 23 2 (set (reg:SI 3 r3 [138])
>         (minus:SI (reg:SI 2 r2 [128])
>             (reg:SI 3 r3 [135]))) x.c:23 720 {thumb1_subsi3_insn}
>      (nil))
>
> However the later pass can recognize the clobber in insn 58 and will remove the
> insn 20 because the r3 defined in insn 20 is clobbered by insn 58.


This is a bug in the arm's thumb1.md file, it uses match_operand when
it should really be using match_scratch.
Looks like most of the arm back-end has this bug in it.

See
https://gcc.gnu.org/onlinedocs/gccint/Regs-and-Memory.html#index-scratch-2870
and the corresponding match_scratch documentation.

That is this:

1515 (define_insn "*cstoresi_eq0_thumb1_insn"
1516   [(set (match_operand:SI 0 "s_register_operand" "=&l,l")
1517         (eq:SI (match_operand:SI 1 "s_register_operand" "l,0")
1518                (const_int 0)))
1519    (clobber (match_operand:SI 2 "s_register_operand" "=X,l"))]
1520   "TARGET_THUMB1"
1521   "@
1522    rsbs\\t%0, %1, #0\;adcs\\t%0, %0, %1
1523    rsbs\\t%2, %1, #0\;adcs\\t%0, %1, %2"
1524   [(set_attr "length" "4")
1525    (set_attr "type" "multiple")]
1526 )

Really should be using (clobber (match_scratch:... ))

So it generates a scratch RTL right away and then the register
allocater knows that it is only used in that instruction.

Thanks,
Andrew

[Bug target/65648] [5 Regression] Bad code due to IRA fails to recognize the clobber in parallel

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Target|                            |arm
           Priority|P3                          |P1
   Target Milestone|---                         |5.0

[Bug target/65648] [5 Regression] Bad code due to IRA fails to recognize the clobber in parallel

[law at redhat dot com]

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

Jeffrey A. Law <law at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |law at redhat dot com
           Assignee|unassigned at gcc dot gnu.org      |vmakarov at redhat dot com

--- Comment #2 from Jeffrey A. Law <law at redhat dot com> ---
Well, I think the real issue here is LRA deciding to move insn 12 regardless of
whether or not the MD file gets a scratch register the recommend way or not.

Note, the insns below are not consecutive in the .ira dump, had me rather
confused for a few minutes.

[Bug target/65648] [5 Regression] Bad code due to IRA fails to recognize the clobber in parallel

[law at redhat dot com]

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

Jeffrey A. Law <law at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2015-04-02
     Ever confirmed|0                           |1

[Bug target/65648] [5 Regression] Bad code due to IRA fails to recognize the clobber in parallel

[hubicka at gcc dot gnu.org]

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

--- Comment #3 from Jan Hubicka <hubicka at gcc dot gnu.org> ---
Author: hubicka
Date: Fri Apr  3 20:25:01 2015
New Revision: 221861

URL: https://gcc.gnu.org/viewcvs?rev=221861&root=gcc&view=rev
Log:

    PR ipa/65648
    * ipa-inline-transform.c (inline_call): Skip sanity check to work
    around the ICE

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/ipa-inline-transform.c

[Bug target/65648] [5 Regression] Bad code due to IRA fails to recognize the clobber in parallel

[vmakarov at gcc dot gnu.org]

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

Vladimir Makarov <vmakarov at gcc dot gnu.org> changed:

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

--- Comment #4 from Vladimir Makarov <vmakarov at gcc dot gnu.org> ---
(In reply to Jeffrey A. Law from comment #2)
> Well, I think the real issue here is LRA deciding to move insn 12 regardless
> of whether or not the MD file gets a scratch register the recommend way or
> not.
> 
> Note, the insns below are not consecutive in the .ira dump, had me rather
> confused for a few minutes.

The insn looks moved as the bug is in new rematerialization sub-pass of LRA.
I'll try to fix this until Wednesday.

[Bug target/65648] [5 Regression] Bad code due to IRA fails to recognize the clobber in parallel

[steven at gcc dot gnu.org]

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

Steven Bosscher <steven at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED

[Bug target/65648] [5 Regression] Bad code due to IRA fails to recognize the clobber in parallel

[jakub at gcc dot gnu.org]

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

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

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

--- Comment #5 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Author: vmakarov
Date: Tue Apr  7 15:01:07 2015
New Revision: 221901

URL: https://gcc.gnu.org/viewcvs?rev=221901&root=gcc&view=rev
Log:
2015-04-07  Vladimir Makarov  <vmakarov@redhat.com>

    PR target/65678
    * lra-remat.c (do_remat): Process input and non-input insn
    registers separately.


Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/lra-remat.c

[Bug target/65648] [5 Regression] Bad code due to IRA fails to recognize the clobber in parallel

[jakub at gcc dot gnu.org]

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

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

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

--- Comment #6 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Thus fixed, still would be nice to have a testcase in the testsuite.  Terry or
other ARM folks, can you please help with that?
See http://gcc.gnu.org/ml/gcc-patches/2015-04/msg00262.html for more details.

[Bug target/65648] [5 Regression] Bad code due to IRA fails to recognize the clobber in parallel

[terry.guo at arm dot com]

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

--- Comment #7 from Terry Guo <terry.guo at arm dot com> ---
(In reply to Jakub Jelinek from comment #6)
> Thus fixed, still would be nice to have a testcase in the testsuite.  Terry
> or other ARM folks, can you please help with that?
> See http://gcc.gnu.org/ml/gcc-patches/2015-04/msg00262.html for more details.

It seems to me that Yvan is working on this, please refer to
https://gcc.gnu.org/ml/gcc-patches/2015-04/msg00272.html.

[Bug target/65648] [5 Regression] Bad code due to IRA fails to recognize the clobber in parallel

[yroux at gcc dot gnu.org]

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

--- Comment #8 from Yvan Roux <yroux at gcc dot gnu.org> ---
Author: yroux
Date: Fri Apr 10 19:03:27 2015
New Revision: 221981

URL: https://gcc.gnu.org/viewcvs?rev=221981&root=gcc&view=rev
Log:
Add missing testcase.

2015-04-19  Yvan Roux  <yvan.roux@linaro.org>

    PR target/65648
    * gcc.target/arm/pr65647-2.c: New.


Added:
    trunk/gcc/testsuite/gcc.target/arm/pr65647-2.c
Modified:
    trunk/gcc/testsuite/ChangeLog

[Bug target/65648] [5 Regression] Bad code due to IRA fails to recognize the clobber in parallel

[yroux at gcc dot gnu.org]

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

--- Comment #9 from Yvan Roux <yroux at gcc dot gnu.org> ---
Author: yroux
Date: Tue Apr 14 12:12:29 2015
New Revision: 222083

URL: https://gcc.gnu.org/viewcvs?rev=222083&root=gcc&view=rev
Log:
Add missing testcase.

2015-04-14  Yvan Roux  <yvan.roux@linaro.org>

    PR target/65648
    * gcc.c-torture/execute/pr65648.c: New test.


Added:
    trunk/gcc/testsuite/gcc.c-torture/execute/pr65648.c
Modified:
    trunk/gcc/testsuite/ChangeLog