[Bug target/40835] New: redundant comparison instruction

Compile the following code with options -Os -mthumb -march=armv5te

int bar();
void goo(int, int);
void foo()
{
  int v = bar();
  if (v == 0)
    return;
  goo(1, v);
}

Gcc generates:

        push    {r3, lr}
        bl      bar
        mov     r1, r0
        cmp     r0, #0    // *
        beq     .L1
        mov     r0, #1
        bl      goo
.L1:
        @ sp needed for prologue
        pop     {r3, pc}

The compare instruction is redundant since the previous move instruction has
already set the condition code according to the value of r0.


--
           Summary: redundant comparison instruction
           Product: gcc
           Version: 4.5.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
        AssignedTo: unassigned at gcc dot gnu dot org
        ReportedBy: carrot at google dot com
 GCC build triplet: i686-linux
  GCC host triplet: i686-linux
GCC target triplet: arm-eabi


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

------- Comment #1 from carrot at google dot com  2009-07-23 08:38 -------
Created an attachment (id=18241)
 --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=18241&action=view)
test case


--


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

------- Comment #2 from carrot at google dot com  2009-07-24 02:11 -------
It seems HAVE_cc0 disabled for arm. What's the reason behind it?

A simple method is to add a peephole rule to handle it.


--


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

------- Comment #3 from steven at gcc dot gnu dot org  2009-07-24 06:59 -------
Because HAVE_cc0 is only for cc0 targets, and ARM is not one of those?

You should stop jumping to peepholes for every missed optimization you find.
There is a csecc pass (part of cse2) that should handle this. You should try to
figure out why that pass can't optimize away your redundant insn.


--


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

------- Comment #4 from carrot at google dot com  2009-07-24 07:37 -------
Just as I've figured out HAVE_cc0 is disabled. And cse_condition_code_reg does
nothing for thumb target.

I also found that the conditional branch instructions is always in the same
insn pattern as the previous compare instructions. So I even wonder there is
any way to express the optimized sequence (movs followed by bcc).

Is there any other places that I should take a look?


--


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

------- Comment #5 from steven at gcc dot gnu dot org  2009-07-24 08:25 -------
The fact that the move sets the condition code is not modelled in the insn.

From .expand dump:
(insn 6 5 7 3 t.c:5 (set (reg/v:SI 133 [ v ])
        (reg:SI 0 r0)) -1 (nil))

From -dAP output:
@(insn 6 5 7 t.c:5 (set (reg/v:SI 1 r1 [orig:133 v ] [133])
@        (reg:SI 0 r0)) 167 {*thumb1_movsi_insn} (nil))
@ 0x0004
        mov     r1, r0  @ 6     *thumb1_movsi_insn/1    [length = 2]

So the compiler never even has a chance.

You could model a new pattern and let combine figure out that the move&compare
can be one insn.  Or you can indeed do a peephole.  The problem with peepholes
is that you may miss opportunities, but adding new patterns also has its
downsides...


--

steven at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
     Ever Confirmed|0                           |1
   Last reconfirmed|0000-00-00 00:00:00         |2009-07-24 08:25:30
               date|                            |


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

------- Comment #6 from steven at gcc dot gnu dot org  2009-07-24 08:48 -------
In fact even the compare isn't a separate insn:

@(insn 6 5 7 t.c:5 (set (reg/v:SI 1 r1 [orig:133 v ] [133])
@        (reg:SI 0 r0)) 167 {*thumb1_movsi_insn} (nil))
@ 0x0004
        mov     r1, r0  @ 6     *thumb1_movsi_insn/1    [length = 2]
@(jump_insn 7 6 8 t.c:6 (set (pc)
@        (if_then_else (eq (reg:SI 0 r0 [orig:133 v ] [133])
@                (const_int 0 [0x0]))
@            (label_ref:SI 14)
@            (pc))) 201 {*cbranchsi4_insn} (expr_list:REG_DEAD (reg:SI 0 r0
[orig:133 v ] [133])
@        (expr_list:REG_BR_PROB (const_int 6102 [0x17d6])
@            (nil))))
@ 0x0006
        cmp     r0, #0  @ 7     *cbranchsi4_insn/1      [length = 4]
        beq     .L1


--


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

------- Comment #7 from rearnsha at gcc dot gnu dot org  2009-07-24 14:08 -------
(In reply to comment #6)
> In fact even the compare isn't a separate insn:

There's a reason for that.

If you separate compares from uses of the flags then reload may end up
inserting add or mov instructions in between the comparison and its use.  Since
thumb1 does not have non-flag setting versions of those instructions (at least
for the lo-regs case) we thus cannot separate the two.

To mitigate this, there are already a number of patterns in the thumb
description that try to exploit flag-setting instructions more efficiently, but
there are bound to be more cases that are not yet handled.  Beware, however, of
generating sequences that are impossible to reload.


--

rearnsha at gcc dot gnu dot org changed:

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


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

------- Comment #8 from rearnsha at gcc dot gnu dot org  2009-11-04 14:10 -------
Subject: Bug 40835

Author: rearnsha
Date: Wed Nov  4 14:09:55 2009
New Revision: 153895

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=153895
Log:
2009-11-04  Richard Earnshaw  <rearnsha@...>

        PR target/40835
        * arm.md (peephole2 patterns for move and compare): New.

2009-11-04  Wei Guozhi  <carrot@...>

        PR target/40835
        * gcc.target/arm/pr40835: New testcase.

Added:
    trunk/gcc/testsuite/gcc.target/arm/pr40835.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/config/arm/arm.md
    trunk/gcc/testsuite/ChangeLog


--


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

------- Comment #9 from carrot at google dot com  2009-11-23 08:51 -------
Fixed by Richard. Close it.


--

carrot at google dot com changed:

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


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