[Bug middle-end/54963] New: Wrong code generated for libgfortran/generated/eoshift3_8.c on SH

[Bug middle-end/54963] New: Wrong code generated for libgfortran/generated/eoshift3_8.c on SH

[kkojima at gcc dot gnu.org]

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

             Bug #: 54963
           Summary: Wrong code generated for
                    libgfortran/generated/eoshift3_8.c on SH
    Classification: Unclassified
           Product: gcc
           Version: 4.8.0
            Status: UNCONFIRMED
          Keywords: wrong-code
          Severity: normal
          Priority: P3
         Component: middle-end
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: kkojima@gcc.gnu.org
            Target: sh*-*-*


Created attachment 28469
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=28469
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.

[Bug target/54963] [4.8 Regression] Wrong code generated for libgfortran/generated/eoshift3_8.c on SH

[kkojima at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |olegendo at gcc dot gnu.org
          Component|middle-end                  |target
            Summary|Wrong code generated for    |[4.8 Regression] Wrong code
                   |libgfortran/generated/eoshi |generated for
                   |ft3_8.c on SH               |libgfortran/generated/eoshi
                   |                            |ft3_8.c on SH

--- Comment #1 from Kazumoto Kojima <kkojima at gcc dot gnu.org> 2012-10-20 06:33:06 UTC ---
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.

[Bug target/54963] [4.8 Regression] Wrong code generated for libgfortran/generated/eoshift3_8.c on SH

[olegendo at gcc dot gnu.org]

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

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

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

--- Comment #2 from Oleg Endo <olegendo at gcc dot gnu.org> 2012-10-20 09:29:10 UTC ---
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.

[Bug target/54963] [4.8 Regression] Wrong code generated for libgfortran/generated/eoshift3_8.c on SH

[olegendo at gcc dot gnu.org]

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

--- Comment #3 from Oleg Endo <olegendo at gcc dot gnu.org> 2012-10-28 23:01:24 UTC ---
Created attachment 28551
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=28551
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.

[Bug target/54963] [4.8 Regression] Wrong code generated for libgfortran/generated/eoshift3_8.c on SH

[kkojima at gcc dot gnu.org]

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

--- Comment #4 from Kazumoto Kojima <kkojima at gcc dot gnu.org> 2012-10-29 00:59:37 UTC ---
(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.

[Bug target/54963] [4.8 Regression] Wrong code generated for libgfortran/generated/eoshift3_8.c on SH

[olegendo at gcc dot gnu.org]

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

--- Comment #5 from Oleg Endo <olegendo at gcc dot gnu.org> 2012-10-29 11:13:19 UTC ---
(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.

[Bug target/54963] [4.8 Regression] Wrong code generated for libgfortran/generated/eoshift3_8.c on SH

[olegendo at gcc dot gnu.org]

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

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

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

--- Comment #6 from Oleg Endo <olegendo at gcc dot gnu.org> 2012-10-30 01:04:10 UTC ---
Created attachment 28567
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=28567
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 ;)

[Bug target/54963] [4.8 Regression] Wrong code generated for libgfortran/generated/eoshift3_8.c on SH

[kkojima at gcc dot gnu.org]

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

--- Comment #7 from Kazumoto Kojima <kkojima at gcc dot gnu.org> 2012-10-30 02:46:09 UTC ---
(In reply to comment #6)

Agreed, patch is pre-approved.

[Bug target/54963] [4.8 Regression] Wrong code generated for libgfortran/generated/eoshift3_8.c on SH

[olegendo at gcc dot gnu.org]

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

--- Comment #8 from Oleg Endo <olegendo at gcc dot gnu.org> 2012-10-30 09:22:31 UTC ---
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

[Bug target/54963] [4.8 Regression] Wrong code generated for libgfortran/generated/eoshift3_8.c on SH

[olegendo at gcc dot gnu.org]

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

--- Comment #9 from Oleg Endo <olegendo at gcc dot gnu.org> 2012-11-01 00:01:13 UTC ---
Kaz, can we close this PR?

[Bug target/54963] [4.8 Regression] Wrong code generated for libgfortran/generated/eoshift3_8.c on SH

[kkojima at gcc dot gnu.org]

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

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

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

--- Comment #10 from Kazumoto Kojima <kkojima at gcc dot gnu.org> 2012-11-01 00:13:13 UTC ---
Yes.  Closed as FIXED.