Created attachment 28469 [details] A test case Several fortran tests using some eoshift functions fail on sh4-unknown-linux-gnu for a while. The above test case is a reduced one which fails sh-elf with -O2 -ml -m4. The right result would be "adhbeh..." but it wrongly returns "ticzzz...". It seems that the compiler allocates a stack slot at r15+60 for "len" variable first for the line 126: len = ((array)->dim[dim]._ubound + 1 - (array)->dim[dim].lower_bound); then computes len - abs (sh) for the line 184; for (n = 0; n < len - delta; n++) using "len" variable at r15+68. The error went away with -fno-ira-share-spill-slots.
It looks that the problematic code run through a wrong flow and the above was misleading. It seems that the change r184829 | olegendo | 2012-03-03 06:21:13 +0900 (Sat, 03 Mar 2012) | 8 lines PR target/49486 * config/sh/sh.md (negdi2): Add TARGET_SH1 condition. (absdi2): New expander. (*absdi2, *negabsdi2, negdi_cond): New insns and splits. * gcc.target/sh/pr49468-si.c: Skip unsupported test for SH64. * gcc.target/sh/pr49468-di.c: New. causes this. After reload, there is an insn for abs(sh): (insn 432 431 1393 39 (set (reg:DI 2 r2) (abs:DI (reg:DI 1 r1))) eoshift.c:167 189 {*absdi2} (nil)) which is splitted into (insn 1607 431 1609 39 (set (reg:SI 147 t) (ge:SI (reg:SI 2 r2 [+4 ]) (const_int 0 [0]))) eoshift.c:167 13 {cmpgesi_t} (nil)) (insn 1609 1607 1610 39 (set (reg:SI 2 r2) (reg:SI 1 r1)) eoshift.c:167 234 {movsi_ie} (nil)) (insn 1610 1609 1611 39 (set (reg:SI 3 r3 [+4 ]) (reg:SI 2 r2 [+4 ])) eoshift.c:167 234 {movsi_ie} (nil)) (jump_insn 1611 1610 1626 39 (set (pc) (if_then_else (ne (reg:SI 147 t) (const_int 0 [0])) ... The insn 1609 changes r2 before its original value is used.
Thanks for tracking this one down. The buggy lines are in the negdi_cond pattern: emit_insn (gen_movsi (low_dst, low_src)); emit_insn (gen_movsi (high_dst, high_src)); I'll have a look at it.
Created attachment 28551 [details] Proposed patch This patch fixes the problem, by using 'emit_move_insn' instead of manually doing the DImode reg copy. I've seized the moment and refactored the abs patterns -- the T_REG clobber handling was a bit confusing and using mode iterators saves a few lines. Kaz, could you please have a look at this one? Only briefly tested with make-gcc and compiling CSiBE. There are no code size changes in the CSiBE set, except for one: jikespg-1.3 src/prntstat 3576 -> 3568 -8 / -0.223714 % I guess it's the T_REG clobber thing that has a positive impact on register allocation in this case.
(In reply to comment #3) > Created attachment 28551 [details] > Proposed patch > > This patch fixes the problem, by using 'emit_move_insn' instead of manually > doing the DImode reg copy. Does the pattern in negdi_cond emit_insn (gen_negc (low_dst, low_src)); emit_label_after (skip_neg_label, emit_insn (gen_negc (high_dst, high_src))); work in the problematic situation? Perhaps I've missed something.
(In reply to comment #4) > (In reply to comment #3) > > Created attachment 28551 [details] > > Proposed patch > > > > This patch fixes the problem, by using 'emit_move_insn' instead of manually > > doing the DImode reg copy. > > Does the pattern in negdi_cond > > emit_insn (gen_negc (low_dst, low_src)); > emit_label_after (skip_neg_label, emit_insn (gen_negc (high_dst, high_src))); > > work in the problematic situation? Perhaps I've missed something. Ugh, you're right. The negdi will go wrong if input and output regs overlap. I guess making the output operand a "=&r" for DImode should fix the issue (as it's done in the adddi3 or subdi3 patterns). Moreover, I think it should be OK to split up the absdi pattern before reload, except for negdi_cond. I'll try that out.
Created attachment 28567 [details] Proposed patch I'm now testing this patch. It fixes the overlapping reg negdi problem. There's a code size increase in CSiBE: linux-2.4.23-pre3-testplatform lib/vsprintf 4776 -> 4820 +44 / +0.921273 % I haven't checked the details of this case, but I guess this is the price of correct code ;)
(In reply to comment #6) Agreed, patch is pre-approved.
Author: olegendo Date: Tue Oct 30 09:22:14 2012 New Revision: 192983 URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=192983 Log: PR target/54963 * config/sh/iterators.md (SIDI): New mode iterator. * config/sh/sh.md (negdi2): Use parallel around operation and T_REG clobber in expander. (*negdi2): Mark output operand as early clobbered. Add T_REG clobber. Split after reload. Simplify split code. (abssi2, absdi2): Fold expanders into abs<mode>2. (*abssi2, *absdi2): Fold into *abs<mode>2 insn_and_split. Split insns before reload. (*negabssi2, *negabsdi2): Fold into *negabs<mode>2. Add T_REG clobber. Split insns before reload. (negsi_cond): Reformat. Use emit_move_insn instead of gen_movesi. (negdi_cond): Reformat. Use emit_move_insn instead of a pair of gen_movsi. Split insn before reload. Modified: trunk/gcc/ChangeLog trunk/gcc/config/sh/iterators.md trunk/gcc/config/sh/sh.md
Kaz, can we close this PR?
Yes. Closed as FIXED.