[Bug target/49468] New: SH Target: inefficient integer abs code

[Bug target/49468] New: SH Target: inefficient integer abs code

[oleg.endo@t-online.de]

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

           Summary: SH Target: inefficient integer abs code
           Product: gcc
           Version: 4.7.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: oleg.endo@t-online.de


The generated code for abs:SI and abs:DI is a bit inefficient:

int abs (int i)
{
  return (i < 0) ? -i : i;
}

mov    r4,r1
shll    r1
subc    r1,r1
mov    r1,r0
xor    r4,r0
rts
sub    r1,r0


long long abs (long long i)
{
  return (i < 0) ? -i : i;
}

mov    r4,r3
shll    r3
subc    r3,r3
mov    r5,r1
xor    r3,r1
mov    r3,r0
clrt
xor    r4,r0
subc    r3,r1
rts    
subc    r3,r0


There is a define_split in sh.md which is supposed to handle the special case
for SH4 but it is not doing anything. The problem has been around since a
couple of GCC 4.x versions. 



sh-elf-gcc -v
Using built-in specs.
COLLECT_GCC=sh-elf-gcc
COLLECT_LTO_WRAPPER=/usr/local/libexec/gcc/sh-elf/4.7.0/lto-wrapper
Target: sh-elf
Configured with: ../gcc-trunk/configure --target=sh-elf --prefix=/usr/local
--enable-languages=c,c++ --enable-multilib --disable-libssp --without-headers
--disable-nls --disable-werror --enable-lto --with-newlib --with-gnu-as
--with-gnu-ld --with-system-zlib : (reconfigured) ../gcc-trunk/configure
--target=sh-elf --prefix=/usr/local --enable-languages=c,c++ --enable-multilib
--disable-libssp --without-headers --disable-nls --disable-werror --enable-lto
--with-newlib --with-gnu-as --with-gnu-ld --with-system-zlib : (reconfigured)
../gcc-trunk/configure --target=sh-elf --prefix=/usr/local
--enable-languages=c,c++ --enable-multilib --disable-libssp --without-headers
--disable-nls --disable-werror --enable-lto --with-newlib --with-gnu-as
--with-gnu-ld --with-system-zlib : (reconfigured) ../gcc-trunk/configure
--target=sh-elf --prefix=/usr/local --enable-multilib --disable-libssp
--without-headers --disable-nls --disable-werror --enable-lto --with-newlib
--with-gnu-as --with-gnu-ld --with-system-zlib target_alias=sh-elf CFLAGS=-Os
CXXFLAGS=-Os --enable-languages=c,c++,lto --no-create --no-recursion
Thread model: single
gcc version 4.7.0 20110619 (experimental) (GCC)

[Bug target/49468] SH Target: inefficient integer abs code

[oleg.endo@t-online.de]

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

--- Comment #1 from Oleg Endo <oleg.endo@t-online.de> 2011-06-19 15:29:45 UTC ---
Created attachment 24560
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=24560
Proposed patch

The patch adds explicit handling of abs:SI and abs:DI to the machine
description instead of relying on the default abs handling. On SH4 the
zero-offset branch way is definitely faster. It reduces pressure on EX group
instructions and improves parallel instruction execution. On other than SH4
zero-offset branches are not as fast, but the resulting code should still be
faster than the default branch-free abs code, at least it is more compact. 
The patch also handles the case of neg (abs (...)) by simply inverting the
branch condition.

Feedback appreciated.

[Bug target/49468] SH Target: inefficient integer abs code

[oleg.endo@t-online.de]

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

--- Comment #2 from Oleg Endo <oleg.endo@t-online.de> 2011-06-19 15:48:41 UTC ---
Created attachment 24561
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=24561
Before/After Examples

[Bug target/49468] SH Target: inefficient integer abs code

[kkojima at gcc dot gnu.org]

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

Kazumoto Kojima <kkojima at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Severity|normal                      |enhancement

--- Comment #3 from Kazumoto Kojima <kkojima at gcc dot gnu.org> 2011-06-22 22:37:28 UTC ---
On sh4-unknown-linux-gnu, this patch causes two new failures on
libstdc++ testsuite

FAIL: 27_io/basic_ostream/inserters_arithmetic/char/7.cc execution test
FAIL: 27_io/basic_ostream/inserters_arithmetic/wchar_t/7.cc execution test

I can't find any differences between generated codes for those
test cases by compilers with/without your patch and the failures
go away if the tests are running with libstdc++ library built
with the unpatched compiler.
So it seems that something in libstdc++ library is miscompiled.
Weired and hard to see what is going on, ATM.

[Bug target/49468] SH Target: inefficient integer abs code

[oleg.endo@t-online.de]

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

Oleg Endo <oleg.endo@t-online.de> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #24560|0                           |1
        is obsolete|                            |

--- Comment #4 from Oleg Endo <oleg.endo@t-online.de> 2011-06-26 20:48:19 UTC ---
Created attachment 24603
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=24603
Proposed patch

Thanks for checking.
I missed to tell the expanders that the T bit is clobbered by insns such as
negdi2 and abssi2 / absdi2. The negdi2 expander that I have changed caused the
libstdc++ tests to fail when formatting a -1LL.

Another (reduced) example:


long long x (long long i, int j, long long k)
{
    if (j & 5)
        return -i;

    return -k;
}

..ended up as (the "j & 5" got lost)...

    mov.l    @(4,r15),r1
    clrt
    mov.l    @r15,r2
    negc    r1,r1
    negc    r2,r0
    bt/s    .L8
    clrt
    negc    r5,r1
    negc    r4,r0
.L8:
    rts    
    nop

[Bug target/49468] SH Target: inefficient integer abs code

[kkojima at gcc dot gnu.org]

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

--- Comment #5 from Kazumoto Kojima <kkojima at gcc dot gnu.org> 2011-06-27 06:39:40 UTC ---
Argh, I also missed clobbers.  Looks fine to me now, except
that insn_and_split "*negdi2" forgot to set constraints and
some minor coding style issues below.

The first comment should be started with a capital letter and
ended with a period.  Also please follow GCC C coding style
even for C program segments in .md file.  C lines in the patch
are started with a tab instead of 2 spaces.  A long conditional
should be broken like as

  (cond
   ? value0
   : value1)

instead of

  (cond ?
         value0 :
         value1)

Please use braces

"
{
  int low_word = ...
  ...

  emit_insn (...
  DONE;
}")

instead of

"
  int low_word = ...
  ...

  emit_insn (...
  DONE;
")

especially when new variables are used, though those braces
aren't required with the current gen* tools.

> +	emit_insn (gen_negsi_cond (operands[0], operands[1], operands[1], 
> +								GEN_INT (1)));

The first line has an extra space after the last comma and
the indentation of the 2nd line doesn't match with GCC coding
standard.  BTW, you could use const[01]_rtx for GEN_INT ([01]):

  emit_insn (gen_negsi_cond (operands[0], operands[1], operands[1],
                 const1_rtx));

There are similar extra white space + broken indentation issues:

> +(define_insn_and_split "negsi_cond"
> +  [(set (match_operand:SI 0 "arith_reg_dest" "=r,r")
> +	(if_then_else:SI (eq:SI (reg:SI T_REG) 
> +							(match_operand:SI 3 "const_int_operand" "M,N"))
...
> +	  emit_label_after (skip_neg_label, 
> +						emit_insn (gen_negsi2 (operands[0], operands[1])));
...

Perhaps mail or editor problem?

[Bug target/49468] SH Target: inefficient integer abs code

[oleg.endo@t-online.de]

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

Oleg Endo <oleg.endo@t-online.de> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #24603|0                           |1
        is obsolete|                            |

--- Comment #6 from Oleg Endo <oleg.endo@t-online.de> 2011-06-29 01:36:46 UTC ---
Created attachment 24625
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=24625
Proposed patch

Added missing constraints to define_insn_and_split.
Fixed the formatting (hopefully ... there seems to be a problem with the tabs.
I'm using 4 spaces wide tabs)

[Bug target/49468] SH Target: inefficient integer abs code

[oleg.endo@t-online.de]

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

Oleg Endo <oleg.endo@t-online.de> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #24625|0                           |1
        is obsolete|                            |

--- Comment #7 from Oleg Endo <oleg.endo@t-online.de> 2011-09-25 12:48:24 UTC ---
Created attachment 25360
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=25360
Proposed patch

The last version of the patch fails the test
gcc.c-torture/execute/arith-rand-ll.c for -m2a-single -mb and multiple
optimization levels with the following error:

internal compiler error: in change_address_1, at emit-rtl.c:1994


The attached version fixes some of the failures but still fails the test above
with -m2a-single -mb -O2. Other optimization levels work fine.

The problem is caused by the define_insn_and_split "*abssi2". It even fails if
the "*abssi2" splits into nothing but a simple register copy (movdi) or
comparison insn. 

I'm now testing the patch without the DI abs parts and will submit it if passes
without new failures.

Cheers,
Oleg

[Bug target/49468] SH Target: inefficient integer abs code

[olegendo at gcc dot gnu.org]

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

olegendo at gcc dot gnu.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |ASSIGNED
   Last reconfirmed|                            |2012-02-26
                 CC|                            |olegendo at gcc dot gnu.org
         AssignedTo|unassigned at gcc dot       |olegendo at gcc dot gnu.org
                   |gnu.org                     |
     Ever Confirmed|0                           |1

[Bug target/49468] SH Target: inefficient integer abs code

[olegendo at gcc dot gnu.org]

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

--- Comment #8 from Oleg Endo <olegendo at gcc dot gnu.org> 2012-02-28 08:41:28 UTC ---
Created attachment 26768
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=26768
Patch to add DImode abs

The attached patch adds DImode abs and did not introduce new failures when
tested against rev 184589.
I'm a bit confused, as this is exactly the same code as I used before to add
DImode abs, and before it was failing in some cases.  Either there was another
bug that got fixed in the middle-end optimizers, or this is just coincidence. 
I've seen that other expanders use force_reg to make sure that operands will be
in regs ... would that be the safer way of doing it?

[Bug target/49468] SH Target: inefficient integer abs code

[kkojima at gcc dot gnu.org]

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

--- Comment #9 from Kazumoto Kojima <kkojima at gcc dot gnu.org> 2012-02-29 23:18:23 UTC ---
(In reply to comment #8)
Perhaps.  Anyway looks fine to me except one minor failure
on sh64-elf:

xsh64-elf-combined/combined/libgcc/libgcc2.c: In function '__powisf2':
xsh64-elf-combined/combined/libgcc/libgcc2.c:1779:1: error: unrecognizable
insn:
(insn 11 10 12 3 (set (reg:DI 170)
        (abs:DI (reg:DI 169)))
xsh64-elf-combined/combined/libgcc/libgcc2.c:1770 -1
     (nil))
xsh64-elf-combined/combined/libgcc/libgcc2.c:1779:1: internal compiler error:
in extract_insn, at recog.c:2123

The failure went away if restricting new absdi2 expander to TARGET_SH1.

--- gcc/config/sh/sh.md~    2012-02-29 10:52:16.000000000 +0900
+++ gcc/config/sh/sh.md    2012-02-29 11:07:42.000000000 +0900
@@ -4538,7 +4538,7 @@ label:
   [(set (match_operand:DI 0 "arith_reg_dest" "")
     (abs:DI (match_operand:DI 1 "arith_reg_operand" "")))
    (clobber (reg:SI T_REG))]
-  ""
+  "TARGET_SH1"
   "")

 (define_insn_and_split "*absdi2"

[Bug target/49468] SH Target: inefficient integer abs code

[olegendo at gcc dot gnu.org]

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

--- Comment #10 from Oleg Endo <olegendo at gcc dot gnu.org> 2012-03-14 22:58:05 UTC ---
Author: olegendo
Date: Wed Mar 14 22:57:58 2012
New Revision: 185397

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=185397
Log:
    PR target/49468
    * gcc.target/sh/pr49468-si.c: Make dg-skip-if not the first directive.
    * gcc.target/sh/pr49468-di.c: Likewise.


Modified:
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/testsuite/gcc.target/sh/pr49468-di.c
    trunk/gcc/testsuite/gcc.target/sh/pr49468-si.c

[Bug target/49468] SH Target: inefficient integer abs code

[olegendo at gcc dot gnu.org]

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

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

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

--- Comment #11 from Oleg Endo <olegendo at gcc dot gnu.org> ---
I think this can be closed.