[Bug target/63347] m68k misoptimisation with -fschedule-insns

[Bug target/63347] m68k misoptimisation with -fschedule-insns

[schwab@linux-m68k.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63347

Andreas Schwab <schwab@linux-m68k.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |WAITING
   Last reconfirmed|                            |2014-09-23
     Ever confirmed|0                           |1
           Severity|major                       |normal

--- Comment #1 from Andreas Schwab <schwab@linux-m68k.org> ---
GCC 4.7 is no longer maintained.  Please try to reproduce it with a newer
version.

[Bug target/63347] m68k misoptimisation with -fschedule-insns

[jifl-bugzilla at jifvik dot org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63347

Jonathan Larmour <jifl-bugzilla at jifvik dot org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
      Known to fail|                            |4.8.3

--- Comment #2 from Jonathan Larmour <jifl-bugzilla at jifvik dot org> ---
(In reply to Andreas Schwab from comment #1)
> GCC 4.7 is no longer maintained.  Please try to reproduce it with a newer
> version.

I can now confirm that it also reproduces equally with gcc 4.8.3. I haven't
tried 4.9 (it was enough of a job getting 4.8 built!).

[Bug target/63347] m68k misoptimisation with -fschedule-insns

[mikpelinux at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63347

--- Comment #3 from Mikael Pettersson <mikpelinux at gmail dot com> ---
I can't reproduce with a vanilla gcc-4.8.3 configured for m68k-elf.  For the
"if (0x0 == haddr) ..." it generates:

        lea (20,%sp),%sp
        tst.l %d2
        seq %d2
        extb.l %d2
        neg.l %d2

which looks correct and matches your 4.7 output.

How was your gcc configured?

[Bug target/63347] m68k misoptimisation with -fschedule-insns

[mikpelinux at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63347

Mikael Pettersson <mikpelinux at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |mikpelinux at gmail dot com

--- Comment #5 from Mikael Pettersson <mikpelinux at gmail dot com> ---
I can reproduce, choosing a CF family cpu causes the needed tstl to disappear.

[Bug target/63347] m68k misoptimisation with -fschedule-insns

[schwab@linux-m68k.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63347

Andreas Schwab <schwab@linux-m68k.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|WAITING                     |NEW

--- Comment #6 from Andreas Schwab <schwab@linux-m68k.org> ---
It disappears in the IRA pass.

[Bug target/63347] m68k misoptimisation with -fschedule-insns

[jifl-bugzilla at jifvik dot org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63347

--- Comment #7 from Jonathan Larmour <jifl-bugzilla at jifvik dot org> ---
I have also now submitted bug 64233 which is about a different testcase which
also gets misoptimised. This may or may not be related, but could well be since
-fschedule-insns is what makes a difference, and a 'tst' disappears (although
there may be more going wrong with that testcase).

Hopefully this will make it easier for someone to work on fixing this, which I
would be very grateful for! Right now, I can't consider the coldfire tools
robust enough to safely use for production code which is unfortunate :-(.

So if anyone could spend some time looking at this bug or bug 64233, that would
be great, thanks!

[Bug target/63347] m68k misoptimisation with -fschedule-insns

[schwab@linux-m68k.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63347

--- Comment #8 from Andreas Schwab <schwab@linux-m68k.org> ---
*** Bug 64233 has been marked as a duplicate of this bug. ***

[Bug target/63347] m68k misoptimisation with -fschedule-insns

[mikpelinux at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63347

--- Comment #9 from Mikael Pettersson <mikpelinux at gmail dot com> ---
This wrong-code started with Bernd's r171845, possibly by exposing a latent
issue.

[Bug target/63347] m68k misoptimisation with -fschedule-insns

[law at redhat dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63347

Jeffrey A. Law <law at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |law at redhat dot com

--- Comment #10 from Jeffrey A. Law <law at redhat dot com> ---
Bernd, need your input on the best direction here...

This BZ is caused by mis-handling of SCHED_GROUP_P in some scheduler work from
a few years ago (meaning it's probably a 4.8 and 4.9 regression as well).

The offending commit is :

Author: bernds <bernds@138bc75d-0d04-0410-961f-82ee72b054a4>
Date:   Fri Apr 1 17:48:35 2011 +0000

        * haifa-sched.c (prune_ready_list): New function, broken out of
        schedule_block.
        (schedule_block): Use it.


    git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/trunk@171845
138bc75d-0d04-0410-961f-82ee72b054a4


As can be seen in this BZ, it can cause incorrect code anywhere we use
SCHED_GROUP_P to keep insns together for correctness.  It can cause
missed-optimizations for things like insn fusion.

The old code would just advance the cycle counter forward the appropriate
number of cycles when it saw a SCHED_GROUP_P insn.   I can hack something
together to have a similar effect here, but I'm unsure if it would break the
tic6x.

[Bug target/63347] [5 regression] m68k misoptimisation with -fschedule-insns

[bernds at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63347

Bernd Schmidt <bernds at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |bernds at gcc dot gnu.org

--- Comment #11 from Bernd Schmidt <bernds at gcc dot gnu.org> ---
(Sorry about the delay, vacation)

Current trunk has SCHED_GROUP_P code in prune_ready_list. That seems to have
been added a year later, so it may have failed to get into a release or two.

Git revisions are (at leat)
  38d8f4bbb3cd8e99a8b8423cc7b19be338a1073a
  ca5e50e427e0401c45bfa93046ace7d3e6719c4b

[Bug target/63347] [5 regression] m68k misoptimisation with -fschedule-insns

[law at redhat dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63347

--- Comment #12 from Jeffrey A. Law <law at redhat dot com> ---
Right.  I know we've got the SCHED_GROUP_P handling in prune_ready_list, but
it's not sufficient.

This is a regression with the trunk.

Take the code from c#0 and run it through a m68k-elf compiler with -O1
-fschedule-insns -mcpu=5208.

Prior to scheduling we have:
(insn 25 24 26 3 (set (cc0)
        (compare (reg/v:SI 32 [ haddr ]) 
            (const_int 0 [0]))) j.c:5 4 {*tstsi_internal_68020_cf}
     (expr_list:REG_DEAD (reg/v:SI 32 [ haddr ])
        (nil)))
(insn 26 25 27 3 (set (reg:QI 54)
        (eq:QI (cc0)
            (const_int 0 [0]))) j.c:5 364 {*m68k.md:5773}
     (nil)) 
(insn 27 26 28 3 (set (reg:SI 53)
        (sign_extend:SI (reg:QI 54))) j.c:5 82 {*68k_extendqisi2}
     (expr_list:REG_DEAD (reg:QI 54)
        (nil)))


After scheduling we get:

(insn 25 24 30 3 (set (cc0)
        (compare (reg/v:SI 32 [ haddr ])
            (const_int 0 [0]))) j.c:5 4 {*tstsi_internal_68020_cf}
     (expr_list:REG_DEAD (reg/v:SI 32 [ haddr ])
        (nil))) 
(insn 30 25 26 3 (set (mem:SI (pre_dec:SI (reg/f:SI 15 %sp)) [0  S4 A16])
        (const_int 10 [0xa])) j.c:17 33 {pushexthisi_const}
     (expr_list:REG_ARGS_SIZE (const_int 4 [0x4])
        (nil)))
(insn 26 30 27 3 (set (reg:QI 54) 
        (eq:QI (cc0)
            (const_int 0 [0]))) j.c:5 364 {*m68k.md:5773}
     (nil))
(insn 27 26 28 3 (set (reg:SI 53)
        (sign_extend:SI (reg:QI 54))) j.c:5 82 {*68k_extendqisi2}
     (expr_list:REG_DEAD (reg:QI 54)
        (nil))) 


Which is obviously wrong with the insertion of insn 30 between the cc-setter
and user.

When I looked at this a month ago my conclusion was that prior to your changes
when we saw the SCHED_GROUP_P flag set, we'd just advance the cycle counter the
appropriate number of cycles to ensure the insn with SCHED_GROUP_P was ready. 

With your changes we schedule the setter, but the result isn't ready for 2
cycles, so we queue the user.  On the next cycle, insn 30 has become ready and
it fires while the user (insn 26) is still queued.  Then 26 issues and we've
got incorrect code.

This history mentions some problem with the old code and the tic6x, but not
enough details for me to know if any changes I might suggest to fix this bug
would in turn mess up the tic6x.

[Bug target/63347] [5 regression] m68k misoptimisation with -fschedule-insns

[bernds at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63347

--- Comment #13 from Bernd Schmidt <bernds at gcc dot gnu.org> ---
Ugh, I see. I think the old method of just advancing should work with c6x -
IIRC SCHED_GROUP_P isn't used there.

[Bug target/63347] [5 regression] m68k misoptimisation with -fschedule-insns

[law at redhat dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63347

Jeffrey A. Law <law at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Assignee|bernds at codesourcery dot com     |law at redhat dot com

--- Comment #14 from Jeffrey A. Law <law at redhat dot com> ---
I think we can fix this by just queueing the SCHED_GROUP_P insn for a single
cycle rather than for its full cost from within prune_ready_list.  That will
cause us to reexamine the ready list each cycle.  If some non-SCHED_GROUP_P
becomes ready before the SCHED_GROUP_P insn is ready, then the
non-SCHED_GROUP_P insn will go back to the queue because it's not part of the
scheduling group.  Which is exactly what we want.

I'm testing with that fix now.

[Bug target/63347] [5 regression] m68k misoptimisation with -fschedule-insns

[law at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63347

--- Comment #15 from Jeffrey A. Law <law at gcc dot gnu.org> ---
Author: law
Date: Wed Feb 11 23:29:11 2015
New Revision: 220632

URL: https://gcc.gnu.org/viewcvs?rev=220632&root=gcc&view=rev
Log:
    PR target/63347
    * haifa-sched.c (prune_ready_list): If we have a SCHED_GROUP_P insn
    that needs to be queued, just queue it for a single cycle.

    PR target/63347
    * gcc.target/m68k/pr63347.c: New test.

Added:
    trunk/gcc/testsuite/gcc.target/m68k/pr63347.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/haifa-sched.c
    trunk/gcc/testsuite/ChangeLog

[Bug target/63347] [4.9 regression] m68k misoptimisation with -fschedule-insns

[law at redhat dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63347

Jeffrey A. Law <law at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|---                         |FIXED
            Summary|[5 regression] m68k         |[4.9 regression] m68k
                   |misoptimisation with        |misoptimisation with
                   |-fschedule-insns            |-fschedule-insns
      Known to fail|                            |4.9.1

--- Comment #16 from Jeffrey A. Law <law at redhat dot com> ---
Fixed on trunk

[Bug target/63347] [4.9 regression] m68k misoptimisation with -fschedule-insns

[law at redhat dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63347

Jeffrey A. Law <law at redhat dot com> changed:

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

--- Comment #17 from Jeffrey A. Law <law at redhat dot com> ---
Sorry, meant to keep this open as a backport would be considered if this issue
were shown to be affecting something other than just m68k

[Bug target/63347] [4.9 regression] m68k misoptimisation with -fschedule-insns

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63347

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
      Known to work|                            |5.0
   Target Milestone|---                         |4.9.3

[Bug target/63347] [4.9 regression] m68k misoptimisation with -fschedule-insns

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63347

--- Comment #18 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
GCC 4.9.3 has been released.

[Bug target/63347] [4.9 regression] m68k misoptimisation with -fschedule-insns

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63347

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|4.9.3                       |4.9.4