[Bug target/62662] New: [4.9/5 Regression] Miscompilation of Qt on s390x

[Bug target/62662] New: [4.9/5 Regression] Miscompilation of Qt on s390x

[jakub at gcc dot gnu.org]

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

            Bug ID: 62662
           Summary: [4.9/5 Regression] Miscompilation of Qt on s390x
           Product: gcc
           Version: 4.9.1
            Status: UNCONFIRMED
          Keywords: wrong-code
          Severity: normal
          Priority: P3
         Component: target
          Assignee: unassigned at gcc dot gnu.org
          Reporter: jakub at gcc dot gnu.org
                CC: krebbel at gcc dot gnu.org, uweigand at gcc dot gnu.org
            Target: s390x-linux

struct A
{
  int a;
  void b ();
};
int c;
struct B
{
  struct C { A d; };
  static C e;
};
struct D
{
  B::C *d;
  D () : d (&B::e) { d->d.b (); }
};
struct F
{
  F (int *);
  int *f;
  D g;
};
void A::b ()
{
  volatile int x;
  __asm__("" : "=&d" (c), "=&d" (x), "=m" (*&a) : "a" (&a), "dm" (0));
}
F::F (int *p1) : f (p1) {}

compiled with -m64 -O2 -fvisibility=hidden -fPIC -march=z9-109 -mtune=z10
results in wrong-code:
        stg     %r15,120(%r15)
        stg     %r3,0(%r2)
.LCFI3:
        lay     %r15,-168(%r15)
.LCFI4:
        larl    %r4,c
        larl    %r1,_ZN1B1eE@GOTENT
        lhi     %r5,0
        lg      %r1,0(%r1)
        stg     %r1,8(%r2)
        st      %r2,0(%r4)
        st      %r3,164(%r15)
        lg      %r4,280(%r15)
        lg      %r15,288(%r15)
.LCFI5:
        br      %r4

The %r14 return register is not saved to the return register stack slot in the
prologue, but is restored from it in the epilogue and jumped to.

>From quick look at this, this seems to be because of bad interaction in between
          /* Fetch return address from stack before load multiple,
             this will do good for scheduling.  */
code in s390_emit_epilogue and s390_optimize_prologue.  If
cfun_frame_layout.save_return_addr_p is false, and first register needing save
during s390_emit_epilogue is below 13 and last above 14, then
s390_emit_epilogue attempts to optimize and loads the return register into
typically %r4 before doing the load multiple insn, but s390_optimize_prologue
doesn't take this into account, it will remove the store multiple insn in the
prologue if at that point only %r15 needs saving, and also replace the load
multiple, but keep loading from return register stack slot into %r4 and return
to %r4.

So, either s390_optimize_prologue needs to adjust also the return insn (and
remove the load of the return address from stack slot if DSE can't handle it?),
or we need to arrange in this case that s390_optimize_prologue doesn't optimize
the return address store away.

[Bug target/62662] [4.9/5 Regression] Miscompilation of Qt on s390x

[jakub at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P3                          |P1
   Target Milestone|---                         |4.9.2

--- Comment #1 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Seems to work fine in 4.8.x, apparently pro_and_epilogue doesn't decide it
needs to save more registers than %r15 in that case.

[Bug target/62662] [4.9/5 Regression] Miscompilation of Qt on s390x

[krebbel at gcc dot gnu.org]

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

Andreas Krebbel <krebbel at gcc dot gnu.org> changed:

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

--- Comment #3 from Andreas Krebbel <krebbel at gcc dot gnu.org> ---
Reghunt indicates that this behavior started with r204752

commit 4bb79f36f65507171b6da263c91e6590334bd342
Author: vmakarov <vmakarov@138bc75d-0d04-0410-961f-82ee72b054a4>
Date:   Wed Nov 13 18:00:43 2013 +0000

    2013-11-13  Vladimir Makarov  <vmakarov@redhat.com>

        PR rtl-optimization/59036
        * ira-color.c (struct allocno_color_data): Add new members
        first_thread_allocno, next_thread_allocno, thread_freq.
        (sorted_copies): New static var.
        (allocnos_conflict_by_live_ranges_p, copy_freq_compare_func): Move
        up.
        (allocno_thread_conflict_p, merge_threads)
        (form_threads_from_copies, form_threads_from_bucket)
        (form_threads_from_colorable_allocno, init_allocno_threads): New
        functions.
        (bucket_allocno_compare_func): Add comparison by thread frequency
        and threads.
        (add_allocno_to_ordered_bucket): Rename to
        add_allocno_to_ordered_colorable_bucket.  Remove parameter.
            (push_only_colorable): Call form_threads_from_bucket.
        (color_pass): Call init_allocno_threads.  Use
        consideration_allocno_bitmap instead of coloring_allocno_bitmap
        for nuillify allocno color data.
        (ira_initiate_assign, ira_finish_assign): Allocate/free
        sorted_copies.
        (coalesce_allocnos): Use static sorted copies.

[Bug target/62662] [4.9/5 Regression] Miscompilation of Qt on s390x

[jakub at gcc dot gnu.org]

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

--- Comment #4 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
That is perhaps where we started to have %r11 clobbered in the routine
(therefore pro_and_epilogue wanted to save %r11-%r15), and all the %r11 uses
later disappear because of later passes (I think cprop_hardreg/rtl_dce mainly).
I'd called that perhaps missed optimization, but not where the bug is, because
even if IRA/LRA is changed not to do it in this case, there is no guarantee it
will not happen on some other code (I mean, the case where during
pro_and_epilogue we see one or more of %r7-%r12 registers clobbered in a leaf
function, but during mach pass not clobbered anymore AND in addition to that
s390_emit_epilogue optimizing and loading return reg from stack slot into
non-%r14 register early before load multiple restore for latency reasons
(presumably)).

IMHO if we consider changing the store multiple instruction in the prologue not
to save %r14, either we need to make sure there are no return insns with
non-%r14 returns (otherwise e.g. save just %r14-%r15 and we can then just
restore %r15), or if there are any, adjust them to return to %r14 instead.

[Bug target/62662] [4.9/5 Regression] Miscompilation of Qt on s390x

[krebbel at gcc dot gnu.org]

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

--- Comment #5 from Andreas Krebbel <krebbel at gcc dot gnu.org> ---
(In reply to Jakub Jelinek from comment #4)
I agree that this is something we need to fix in the back-end. I was just
curious about when this surfaced first and keep that info for the records.

[Bug target/62662] [4.9/5 Regression] Miscompilation of Qt on s390x

[krebbel at gcc dot gnu.org]

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

--- Comment #6 from Andreas Krebbel <krebbel at gcc dot gnu.org> ---
Created attachment 33450
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=33450&action=edit
Experimental patch

I'm currently bootstrapping this patch with various combinations. It will take
some time.

[Bug target/62662] [4.9/5 Regression] Miscompilation of Qt on s390x

[krebbel at gcc dot gnu.org]

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

--- Comment #7 from Andreas Krebbel <krebbel at gcc dot gnu.org> ---
Author: krebbel
Date: Fri Sep 19 09:14:59 2014
New Revision: 215381

URL: https://gcc.gnu.org/viewcvs?rev=215381&root=gcc&view=rev
Log:
2014-09-19  Andreas Krebbel  <Andreas.Krebbel@de.ibm.com>

    PR target/62662
    * config/s390/s390.c (s390_emit_epilogue): When doing the return
    address load optimization force s390_optimize_prologue to leave it
    that way.  Only do the optimization if we already decided to push
    r14 into a stack slot.


Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/config/s390/s390.c

[Bug target/62662] [4.9/5 Regression] Miscompilation of Qt on s390x

[krebbel at gcc dot gnu.org]

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

--- Comment #8 from Andreas Krebbel <krebbel at gcc dot gnu.org> ---
Author: krebbel
Date: Fri Sep 19 09:20:38 2014
New Revision: 215383

URL: https://gcc.gnu.org/viewcvs?rev=215383&root=gcc&view=rev
Log:
2014-09-19  Andreas Krebbel  <Andreas.Krebbel@de.ibm.com>

    PR target/62662
    * config/s390/s390.c (s390_emit_epilogue): When doing the return
    address load optimization force s390_optimize_prologue to leave it
    that way.  Only do the optimization if we already decided to push
    r14 into a stack slot.


Modified:
    branches/gcc-4_9-branch/gcc/config/s390/s390.c

[Bug target/62662] [4.9/5 Regression] Miscompilation of Qt on s390x

[jakub at gcc dot gnu.org]

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

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

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

--- Comment #9 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Fixed, thanks.