[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.
