[Bug target/60431] New: [PATCH] [TIC6X] target description missing abssi2 insn

[Bug target/60431] New: [PATCH] [TIC6X] target description missing abssi2 insn

[wojtek.golf at interia dot pl]

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

            Bug ID: 60431
           Summary: [PATCH] [TIC6X] target description missing abssi2 insn
           Product: gcc
           Version: 4.8.3
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
          Assignee: unassigned at gcc dot gnu.org
          Reporter: wojtek.golf at interia dot pl
              Host: Linux wmigda-desktop 3.11.0-13-generic #20-Ubuntu SMP
                    Wed Oct 23 17:26:33 UTC 2013 i686 i686 i686 GNU/Linux
            Target: tic6x-none-elf
             Build: tic6x-none-elf-gcc-4.8.3 (GCC) 4.8.3 20140303
                    (prerelease)

Target definition has ssabssi2 insn definition, which uses ss_abs operand, but
does not have an insn which uses abs operand. As a result simple code which one
would expect to generate abs insn generates much less efficient set of
expressions.

For the testcase:

# 1 "main.c"
# 1 "<command-line>"
# 1 "main.c"
int fn_1(int x)
{
  return (x >= 0) ? x : -x;
}

int fn_2(int x)
{
  return (x > 0) ? x : -x;
}

stock gcc generates (-march=c674x -fverbose-asm -O2 -g0 -S -dp main.c
-fdump-final-insns=rtl-dump.rtl):

(insn# 0 0 2 (sequence [
            (insn:TI# # # 2 (unspec [
                        (reg:SI 35 B3)
                        (const_int 2 [0x2])
                    ] UNSPEC_REAL_JUMP) main.c:4# {real_ret}
                 (nil))
            (insn# # # 2 (set (reg:SI 3 A3 [77])
                    (ashiftrt:SI (reg/v:SI 4 A4 [orig:75 x ] [75])
                        (const_int 31 [0x1f]))) main.c:4# {ashrsi3}
                 (nil))
        ]) main.c:4#
     (nil))
(insn:TI# 0 0 2 (set (reg:SI 4 A4 [orig:76 D.1411 ] [76])
        (xor:SI (reg:SI 3 A3 [77])
            (reg/v:SI 4 A4 [orig:75 x ] [75]))) main.c:3# {xorsi3}
     (nil))
(insn:TI# 0 0 2 (set (reg/i:SI 4 A4)
        (minus:SI (reg:SI 4 A4 [orig:76 D.1411 ] [76])
            (reg:SI 3 A3 [77]))) main.c:4# {subsi3}
     (expr_list:REG_DEAD (reg:SI 3 A3 [77])
        (nil)))
(insn:TI# 0 0 2 (unspec [
            (const_int 3 [0x3])
        ] UNSPEC_NOP) main.c:4# {nop_count}
     (nil))
(insn# 0 0 2 (use (reg/i:SI 4 A4)) main.c:4#
     (nil))
(jump_insn:TI# 0 0 2 (parallel [
            (unspec [
                    (const_int 0 [0])
                ] UNSPEC_JUMP_SHADOW)
            (return)
        ]) main.c:4# {return_shadow}
     (expr_list:REG_DEAD (reg:SI 35 B3)
        (nil))
 -> return)


In contrast, Texas Instruments compiler produces the machine code below:

_fn_1:
           BNOP.S2       B3,4
           ABS.L1        A4,A4
_fn_2:
           BNOP.S2       B3,3
           CMPLT.L1      0,A4,A0
    [!A0]  NEG.L1        A4,A4

The proposed patch introduces a clone of the ssabssi2 insn which uses abs
instead of ss_abs operand.
When it is applied gcc produces rtl below:

(insn# 0 0 2 (sequence [
            (insn:TI# # # 2 (unspec [
                        (reg:SI 35 B3)
                        (const_int 2 [0x2])
                    ] UNSPEC_REAL_JUMP) main.c:4# {real_ret}
                 (nil))
            (insn# # # 2 (set (reg/i:SI 4 A4)
                    (abs:SI (reg:SI 4 A4 [ x ]))) main.c:4# {abssi2}
                 (nil))
        ]) main.c:4#
     (nil))
(insn:TI# 0 0 2 (unspec [
            (const_int 5 [0x5])
        ] UNSPEC_NOP) main.c:4# {nop_count}
     (nil))
(insn# 0 0 2 (use (reg/i:SI 4 A4)) main.c:4#
     (nil))
(jump_insn:TI# 0 0 2 (parallel [
            (unspec [
                    (const_int 0 [0])
                ] UNSPEC_JUMP_SHADOW)
            (return)
        ]) main.c:4# {return_shadow}
     (expr_list:REG_DEAD (reg:SI 35 B3)
        (nil))
 -> return)

for both functions fn_1 and fn_2 from the testcase listed above. One can notice
that this time abssi2 will be used and that for fn_2 gcc will properly
recognize that it can generate the same rtl as for fn_1, which will be better
than that produced by the TI compiler. 

ChangeLog
2014-03-05  Wojciech Migda  <wojtek.golf@interia.pl>

    * gcc/config/c6x/c6x.md: abssi2 instruction.
    * gcc/testsuite/gcc.target/tic6x/abssi2-scan.c: abssi2 testcases.

Bootstrapping and testing
Host: Linux wmigda-desktop 3.11.0-13-generic #20-Ubuntu SMP Wed Oct 23 17:26:33
UTC 2013 i686 i686 i686 GNU/Linux
Target: tic6x-none-elf
Results for the new testcases (run with make check-gcc
RUNTESTFLAGS="CFLAGS_FOR_TARGET='$CFLAGS_FOR_TARGET
--sysroot=${CXTOOLS}${TRIPLET}/sysroot' -v -v tic6x.exp")

PASS: gcc.target/tic6x/abssi2-scan.c (test for excess errors)
PASS: gcc.target/tic6x/abssi2-scan.c scan-assembler-times [\\t ]abs[\\t ] 2

Patch attached.

[Bug target/60431] [PATCH] [TIC6X] target description missing abssi2 insn

[wojtek.golf at interia dot pl]

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

--- Comment #1 from Wojciech Migda <wojtek.golf at interia dot pl> ---
Created attachment 32275
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=32275&action=edit
Proposed patch

[Bug target/60431] [PATCH] [TIC6X] target description missing abssi2 insn

[wojtek.golf at interia dot pl]

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

--- Comment #2 from Wojciech Migda <wojtek.golf at interia dot pl> ---
Created attachment 32276
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=32276&action=edit
patch amendment - previous was incomplete

[Bug target/60431] [PATCH] [TIC6X] target description missing abssi2 insn

[joseph at codesourcery dot com]

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

--- Comment #3 from joseph at codesourcery dot com <joseph at codesourcery dot com> ---
The semantics of the abssi2 insn pattern on the most negative integer are 
that it returns the argument unchanged (RTL operations are generally 
modulo; the semantics don't depend on whether -fwrapv is used).  Thus, a 
saturating abs instruction is unsuitable for implementing that pattern; 
you need to make the machine-independent compiler able to use ssabssi2 to 
expand ABS_EXPR-with-undefined-overflow (i.e. ABS_EXPR without flag_wrapv 
or flag_trapv), when the saturating pattern is present but not the 
non-saturating pattern.

[Bug target/60431] [PATCH] [TIC6X] target description missing abssi2 insn

[wojtek.golf at interia dot pl]

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

--- Comment #4 from Wojciech Migda <wojtek.golf at interia dot pl> ---
Ok, my bad. I'll work on the testcases so that I can make gcc emit abs when it
is valid. Should this PR be rejected then?

[Bug target/60431] [PATCH] [TIC6X] target description missing abssi2 insn

[wojtek.golf at interia dot pl]

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

--- Comment #5 from Wojciech Migda <wojtek.golf at interia dot pl> ---
So, I'd like to learn some more from this. Suppose I have code like this:

int fn_i1(int x)
{
  x /= 2;
  return (x >= 0) ? x : -x;
}

or

short fn_s1(short x)
{
  return (x >= 0) ? x : -x;
}

In each case the compiler should know that the range of values being subjected
to *abs is such that it won't matter whether it will use the sign saturated
instruction or not. But it doesn't take advantage of that. Or I am still
getting it wrong?

[Bug target/60431] [PATCH] [TIC6X] target description missing abssi2 insn

[joseph at codesourcery dot com]

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

--- Comment #6 from joseph at codesourcery dot com <joseph at codesourcery dot com> ---
That sounds like another case for adding an target-independent feature.  
That is, the target description is accurate here but the 
target-independent parts of the compiler could do with new features to 
optimize better for such targets.

[Bug target/60431] [PATCH] [TIC6X] target description missing abssi2 insn

[wojtek.golf at interia dot pl]

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

Wojciech Migda <wojtek.golf at interia dot pl> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |RESOLVED
         Resolution|---                         |INVALID

--- Comment #7 from Wojciech Migda <wojtek.golf at interia dot pl> ---
misread documentation.