Bug 70025 - [6 Regression] Miscompilation of gc-7.4.2 on s390x starting with r227382
Summary: [6 Regression] Miscompilation of gc-7.4.2 on s390x starting with r227382
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: middle-end (show other bugs)
Version: 6.0
: P1 normal
Target Milestone: 6.0
Assignee: Not yet assigned to anyone
URL:
Keywords: wrong-code
Depends on:
Blocks:
 
Reported: 2016-03-01 10:04 UTC by Jakub Jelinek
Modified: 2016-03-02 07:34 UTC (History)
3 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2016-03-01 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Jakub Jelinek 2016-03-01 10:04:30 UTC
typedef char (*F) (unsigned long, void *);
typedef union { struct A { char a1, a2, a3, a4; unsigned long a5; F a6; void *a7; } b; char c[1]; } B;
struct C { const char *c1; unsigned long c2; };
typedef struct D { unsigned long d1; int d2; const char *d3; unsigned long d4, d5; struct C d6[49]; char d7[8]; } E[1];

__attribute__ ((noinline, noclone))
void foo (register E p)
{
  asm volatile (""::"r" (p):"memory");
}

__attribute__ ((noinline, noclone))
void bar (register E p)
{
  register unsigned long k = p[0].d1 + 1;
  register struct C *l = &((p)[0].d6[(p)[0].d2]);
  register const char *m = l->c1;
  p[0].d1 = k;
  if (*m == '\0')
    {
      register struct A *f = &((B *) m)->b;
      register unsigned long n = l->c2;
      register unsigned long o = n + f->a5;
      if (k < o)
	{
	  register unsigned long i;
	  register unsigned long q = k + 8;
	  register F a6 = f->a6;
	  register void *a7 = f->a7;
	  if (q > o)
	    q = o;
	  for (i = k; i < q; i++)
	    p[0].d7[i - k] =
	      (*a6) (i - n, a7);
	  p[0].d4 = k;
	  p[0].d3 = p[0].d7;
	  p[0].d5 = q;
	  return;
	}
    }
  while (p[0].d2 > 0 && l[0].c2 != l[-1].c2)
    {
      p[0].d2--;
      l--;
    }
  if (p[0].d2 == 0)
    {
      p[0].d2 = 0x55555555;
      return;
    }
  p[0].d2--;
  foo (p);
}

char
baz (unsigned long i, void *j)
{
  if (j != 0)
    __builtin_abort ();
  return (char) i;
}

int
main ()
{
  struct D p;
  struct A f;
  __builtin_memset (&f, 0, sizeof (f));
  f.a2 = 4;
  f.a5 = 13;
  f.a6 = baz;
  __builtin_memset (&p, 0, sizeof (p));
  p.d6[0].c1 = (const char *) &f;
  bar (&p);
  if (p.d4 != 1 || p.d5 != 9 || p.d3 != p.d7)
    __builtin_abort ();
  return 0;
}

is miscompiled on s390x-linux, starting with r227382 with -m64 -march=z9-109 -mtune=z10 -O2.
In *.ira we have:
(insn 97 98 191 10 (parallel [
            (set (mem/f:DI (plus:DI (reg/v/f:DI 151 [orig:129 p ] [129])
                        (const_int 16 [0x10])) [4 p_8(D)->d3+0 S8 A64])
                (plus:DI (reg/v/f:DI 151 [orig:129 p ] [129])
                    (const_int 824 [0x338])))
            (clobber (reg:CC 33 %cc))
        ]) cordbscs2.c:36 1267 {*adddi3}
     (expr_list:REG_DEAD (reg/v/f:DI 151 [orig:129 p ] [129])
        (expr_list:REG_UNUSED (reg:CC 33 %cc)
            (nil))))
but LRA incorrectly reloads it as:
(insn 97 98 224 10 (parallel [
            (set (reg/v/f:DI 1 %r1 [orig:129 p ] [129])
                (plus:DI (reg/v/f:DI 1 %r1 [orig:129 p ] [129])
                    (const_int 824 [0x338])))
            (clobber (reg:CC 33 %cc))
        ]) cordbscs2.c:36 1267 {*adddi3}
     (nil))
(insn 224 97 191 10 (set (mem/f:DI (plus:DI (reg/v/f:DI 1 %r1 [orig:129 p ] [129])
                (const_int 16 [0x10])) [4 p_8(D)->d3+0 S8 A64])
        (reg/v/f:DI 1 %r1 [orig:129 p ] [129])) cordbscs2.c:36 995 {*movdi_64}
     (nil))
Thus it writes correct value to a wrong address.
Comment 1 Jakub Jelinek 2016-03-01 10:06:54 UTC
Neither r228097 nor r228153 helps here, it is reproduceable on current trunk too.
Comment 2 Dominik Vogt 2016-03-01 10:49:34 UTC
This is related to https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61578
Comment 3 Dominik Vogt 2016-03-01 10:54:04 UTC
Looks like the extra condition in that patch is still not good enough:

--- a/gcc/lra-constraints.c
+++ b/gcc/lra-constraints.c
@@ -945,6 +945,12 @@ match_reload (signed char out, signed char *ins, enum reg_c
       	= (ins[1] < 0 && REG_P (in_rtx)
           && (int) REGNO (in_rtx) < lra_new_regno_start
           && find_regno_note (curr_insn, REG_DEAD, REGNO (in_rtx))
+          /* We can not use the same value if the pseudo is mentioned
+             in the output, e.g. as an address part in memory,
+             becuase output reload will actually extend the pseudo
+             liveness.  We don't care about eliminable hard regs here
+             as we are interesting only in pseudos.  */
+          && (out < 0 || regno_use_in (REGNO (in_rtx), out_rtx) == NULL_RTX)
           ? lra_create_new_reg (inmode, in_rtx, goal_class, "")
           : lra_create_new_reg_with_unique_value (outmode, out_rtx,
                                                   goal_class, ""));
Comment 4 Jakub Jelinek 2016-03-01 11:27:56 UTC
Indeed.  At this point we have:
(insn 97 98 191 10 (parallel [
            (set (mem/f:DI (plus:DI (reg/v/f:DI 164 [orig:129 p ] [129])
                        (const_int 16 [0x10])) [4 p_8(D)->d3+0 S8 A64])
                (plus:DI (reg/v/f:DI 151 [orig:129 p ] [129])
                    (const_int 824 [0x338])))
            (clobber (reg:CC 33 %cc))
        ]) cordbscs2.c:36 1267 {*adddi3}
     (expr_list:REG_DEAD (reg/v/f:DI 151 [orig:129 p ] [129])
        (expr_list:REG_UNUSED (reg:CC 33 %cc)
            (nil))))
where the pseudo 164 has been created by process_addr_reg earlier and the former pseudo 151 at that spot changed to the pseudo 164.
Comment 5 Dominik Vogt 2016-03-01 11:53:18 UTC
Yup.

  debug_rtx(out_rtx) = (mem/f:DI (plus:DI (reg/v/f:DI 164 [orig:129 p ] [129])
          (const_int 16 [0x10])) [4 p_8(D)->d3+0 S8 A64])

  debug_rtx(in_rtx) = (reg/v/f:DI 151 [orig:129 p ] [129])

Because in_rtx doesn't appear in out_rtx the condition "regno_use_in (REGNO (in_rtx), out_rtx) == 0" misses its mark.
Comment 6 Dominik Vogt 2016-03-01 13:34:07 UTC
Shouldn't this rather check whether the *value* of the register in in_rtx appears in out_rtx?
Comment 7 Vladimir Makarov 2016-03-02 01:40:03 UTC
Author: vmakarov
Date: Wed Mar  2 01:39:30 2016
New Revision: 233876

URL: https://gcc.gnu.org/viewcvs?rev=233876&root=gcc&view=rev
Log:
2016-03-01  Vladimir Makarov  <vmakarov@redhat.com>

	PR middle-end/70025
	* lra-constraints.c (regno_val_use_in): New.
	(match_reload): Use it instead of regno_use_in.


Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/lra-constraints.c
Comment 8 Jakub Jelinek 2016-03-02 06:58:37 UTC
Author: jakub
Date: Wed Mar  2 06:58:05 2016
New Revision: 233889

URL: https://gcc.gnu.org/viewcvs?rev=233889&root=gcc&view=rev
Log:
	PR middle-end/70025
	* gcc.dg/torture/pr70025.c: New test.

Added:
    trunk/gcc/testsuite/gcc.dg/torture/pr70025.c
Modified:
    trunk/gcc/testsuite/ChangeLog
Comment 9 Jakub Jelinek 2016-03-02 07:04:08 UTC
Fixed.
Comment 10 Dominik Vogt 2016-03-02 07:34:08 UTC
Successfully bootstrapped and regression tested on s390x (-m31 and -m64).