[Bug target/49468] SH Target: inefficient integer abs code
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49468 Oleg Endo changed: What|Removed |Added Status|ASSIGNED|RESOLVED Resolution|--- |FIXED --- Comment #11 from Oleg Endo --- I think this can be closed.
[Bug target/49468] SH Target: inefficient integer abs code
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49468 --- Comment #10 from Oleg Endo 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
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49468 --- Comment #9 from Kazumoto Kojima 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.0 +0900 +++ gcc/config/sh/sh.md2012-02-29 11:07:42.0 +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
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49468 --- Comment #8 from Oleg Endo 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
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
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49468 Oleg Endo changed: What|Removed |Added Attachment #24625|0 |1 is obsolete|| --- Comment #7 from Oleg Endo 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
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49468 Oleg Endo changed: What|Removed |Added Attachment #24603|0 |1 is obsolete|| --- Comment #6 from Oleg Endo 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
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49468 --- Comment #5 from Kazumoto Kojima 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
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49468 Oleg Endo changed: What|Removed |Added Attachment #24560|0 |1 is obsolete|| --- Comment #4 from Oleg Endo 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 negcr1,r1 negcr2,r0 bt/s.L8 clrt negcr5,r1 negcr4,r0 .L8: rts nop
[Bug target/49468] SH Target: inefficient integer abs code
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49468 Kazumoto Kojima changed: What|Removed |Added Severity|normal |enhancement --- Comment #3 from Kazumoto Kojima 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
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49468 --- Comment #2 from Oleg Endo 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
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49468 --- Comment #1 from Oleg Endo 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.