[Bug target/54685] [SH] Improve unsigned int comparison with 0x7FFFFFFF

[Bug target/54685] [SH] Improve unsigned int comparison with 0x7FFFFFFF

[olegendo at gcc dot gnu.org]

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

Oleg Endo <olegendo at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |ASSIGNED
   Last reconfirmed|                            |2012-09-23
     Ever Confirmed|0                           |1

[Bug target/54685] New: [SH] Improve unsigned int comparison with 0x7FFFFFFF

[olegendo at gcc dot gnu.org]

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

             Bug #: 54685
           Summary: [SH] Improve unsigned int comparison with 0x7FFFFFFF
    Classification: Unclassified
           Product: gcc
           Version: 4.8.0
            Status: UNCONFIRMED
          Severity: enhancement
          Priority: P3
         Component: target
        AssignedTo: olegendo@gcc.gnu.org
        ReportedBy: olegendo@gcc.gnu.org
            Target: sh*-*-*


The following cases:


int test_00 (unsigned int a)
{
  return !(a > 0x7FFFFFFF);
}


int test_01 (unsigned int a)
{
  return a <= 0x7FFFFFFF;
}

int test_02 (unsigned int a)
{
  return a < 0x80000000;
}

compile to (-m4 -O2):
        not     r4,r0
        shll    r0
        rts
        movt    r0

which would be better as:
        cmp/pz    r4    ! T = r4 >= 0 (signed)
        rts
        movt    r0


For some reason, these do not go through the cstoresi4 expander, but the
following does:

int test (unsigned int a)
{
  return a > 0;
}

The cstore is expanded by the middle-end into something like

(insn 6 3 7 2 (set (reg:SI 167)
        (not:SI (reg/v:SI 164 [ a ]))) sh_tmp.cpp:1984 185 {one_cmplsi2}
     (nil))
(insn 7 6 8 2 (parallel [
            (set (reg:SI 166 [ D.42391+-3 ])
                (lshiftrt:SI (reg:SI 167)
                    (const_int 31 [0x1f])))
            (clobber (reg:SI 147 t))
        ]) sh_tmp.cpp:1984 167 {lshrsi3_n_clobbers_t}
     (nil))


And combine then looks for:

Failed to match this instruction:
(parallel [
        (set (reg:SI 166 [ D.42391+-3 ])
            (ge:SI (reg:SI 4 r4 [ a ])
                (const_int 0 [0])))
        (clobber (reg:SI 147 t))
    ])

[Bug target/54685] [SH] Improve unsigned int comparison with 0x7FFFFFFF

[olegendo at gcc dot gnu.org]

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

--- Comment #1 from Oleg Endo <olegendo at gcc dot gnu.org> 2012-10-04 23:50:16 UTC ---
Just for the record...
I've tried adding this pattern

Failed to match this instruction:
(parallel [
        (set (reg:SI 166 [ D.42391+-3 ])
            (ge:SI (reg:SI 4 r4 [ a ])
                (const_int 0 [0])))
        (clobber (reg:SI 147 t))
    ])

It fixes the test cases in the description, but prevents some of the div0s
integer sign comparison patterns from matching, since combine would try out
different combinations with the pattern above.

[Bug target/54685] [SH] Improve unsigned int comparison with 0x7FFFFFFF

[olegendo at gcc dot gnu.org]

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

--- Comment #2 from Oleg Endo <olegendo at gcc dot gnu.org> 2012-10-08 10:09:34 UTC ---
Author: olegendo
Date: Mon Oct  8 10:09:28 2012
New Revision: 192200

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=192200
Log:
    PR target/54685
    * config/sh/sh.md (one_cmplsi2): Make insn_and_split.  Add manual
    combine matching for an insn sequence where a ge:SI pattern can be used.

    PR target/54685
    * gcc.target/sh/pr54685.c: New.


Added:
    trunk/gcc/testsuite/gcc.target/sh/pr54685.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/config/sh/sh.md
    trunk/gcc/testsuite/ChangeLog

[Bug middle-end/54685] [SH] Improve unsigned int comparison with 0x7FFFFFFF

[olegendo at gcc dot gnu.org]

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

Oleg Endo <olegendo at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
          Component|target                      |middle-end

--- Comment #3 from Oleg Endo <olegendo at gcc dot gnu.org> 2012-10-08 18:36:14 UTC ---
The symptoms on SH have been fixed by the patch in rev 192200.
I'd like to change this to a middle-end issue, since the actual problem seems
to be in the initial expansion of the comparison.  I haven't checked it out but
it might also be a problem on other targets.

[Bug middle-end/54685] [SH] Improve unsigned int comparison with 0x7FFFFFFF

[olegendo at gcc dot gnu.org]

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

Oleg Endo <olegendo at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |SUSPENDED

--- Comment #4 from Oleg Endo <olegendo at gcc dot gnu.org> 2012-11-06 20:24:57 UTC ---
I've briefly checked this on ARM and there the expansion is done in a similar
way.  However, due to ARM's ISA it is not that much of a problem.

[Bug middle-end/54685] [SH] Improve unsigned int comparison with 0x7FFFFFFF

[olegendo at gcc dot gnu.org]

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

Oleg Endo <olegendo at gcc dot gnu.org> changed:

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

--- Comment #5 from Oleg Endo <olegendo at gcc dot gnu.org> 2013-02-04 23:03:15 UTC ---
Kaz,

I've noticed that in your sh4-unknown-linux-gnu test results, the test for this
PR fails:

        === gcc tests ===

FAIL: gcc.target/sh/pr54685.c scan-assembler-not not

I'm curious why this fails.  On my sh-elf / newlib config it passes.  Do you
have any idea?

[Bug middle-end/54685] [SH] Improve unsigned int comparison with 0x7FFFFFFF

[kkojima at gcc dot gnu.org]

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

--- Comment #6 from Kazumoto Kojima <kkojima at gcc dot gnu.org> 2013-02-05 00:09:01 UTC ---
(In reply to comment #5)
> FAIL: gcc.target/sh/pr54685.c scan-assembler-not not
> 
> I'm curious why this fails.  On my sh-elf / newlib config it passes.  Do you
> have any idea?

Oops.  There is a line

    .section    .note.GNU-stack,"",@progbits

for the linux case.  Perhaps

/* { dg-final { scan-assembler-not "not\[ \t\]" } } */

will work.

[Bug middle-end/54685] [SH] Improve unsigned int comparison with 0x7FFFFFFF

[olegendo at gcc dot gnu.org]

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

--- Comment #7 from Oleg Endo <olegendo at gcc dot gnu.org> 2013-02-15 21:28:32 UTC ---
Author: olegendo
Date: Fri Feb 15 21:28:26 2013
New Revision: 196092

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=196092
Log:
    PR target/54685
    * gcc.target/sh/pr54685.c: Fix scanning of not insn.


Modified:
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/testsuite/gcc.target/sh/pr54685.c

[Bug middle-end/54685] [SH] Improve unsigned int comparison with 0x7FFFFFFF

[olegendo at gcc dot gnu.org]

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

--- Comment #8 from Oleg Endo <olegendo at gcc dot gnu.org> 2013-02-15 21:33:45 UTC ---
(In reply to comment #6)
> (In reply to comment #5)
> > FAIL: gcc.target/sh/pr54685.c scan-assembler-not not
> > 
> > I'm curious why this fails.  On my sh-elf / newlib config it passes.  Do you
> > have any idea?
> 
> Oops.  There is a line
> 
>     .section    .note.GNU-stack,"",@progbits
> 
> for the linux case.  Perhaps
> 
> /* { dg-final { scan-assembler-not "not\[ \t\]" } } */
> 
> will work.

I suspected it to be something like that.  Thanks for checking.

[Bug middle-end/54685] [SH] Improve unsigned int comparison with 0x7FFFFFFF

[olegendo at gcc dot gnu.org]

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

--- Comment #9 from Oleg Endo <olegendo at gcc dot gnu.org> ---
This is basically the same issue as PR 59533.  emit_store_flag_1 in expmed.c
always expands the not-shift because the assumption there is that it's cheaper,
which is not true for SH.

The pre-peephole idea from http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59533#c2
also fixes this problem and makes the change in r192200 superfluous.