Bug 36533 - [4.3/4.4 Regression] Incorrectly assumed aligned_operand
Summary: [4.3/4.4 Regression] Incorrectly assumed aligned_operand
Alias: None
Product: gcc
Classification: Unclassified
Component: target (show other bugs)
Version: 4.3.1
: P1 normal
Target Milestone: 4.3.2
Assignee: Not yet assigned to anyone
Keywords: wrong-code
Depends on:
Reported: 2008-06-13 21:31 UTC by Jakub Jelinek
Modified: 2008-06-23 13:19 UTC (History)
5 users (show)

See Also:
Target: i686-linux
Known to work:
Known to fail:
Last reconfirmed:


Note You need to log in before you can comment on or make changes to this bug.
Description Jakub Jelinek 2008-06-13 21:31:47 UTC
The following testcase is miscompiled on i?86 -m32 -Os:

/* { dg-options "-Os" } */

typedef struct S1
  unsigned long s1;
  struct S1 *s2;
  char *s3;
} S1;

typedef struct
  unsigned int s4;
  unsigned int s5;
  int s6;
  unsigned int *s7;
} S2;

typedef struct
  unsigned int s8;
  unsigned short s9;
  unsigned char s10;
  unsigned char s11;
  char s12[255];
} S3;

typedef struct
  unsigned int s4;
  unsigned short s13;
  unsigned short s14;
} S4;

typedef struct
  char s15[16];
  unsigned long s16;
} S5;

typedef struct
  char s15[48];
  S5 *s17;
} S6;

typedef struct
  S1 *s18;
} S7;

extern __attribute__((regparm (3))) int fn1 (const char *x, void *y, S1 *z);
extern __attribute__((regparm (3))) int fn2 (const char *x, int y, S2 *z);

static inline __attribute__ ((always_inline)) unsigned int
fn4 (unsigned short x)
  unsigned len = x;
  if (len == ((1 << 16) - 1))
    return 1 << 16;
  return len;

static inline __attribute__ ((always_inline)) S3 *
fn3 (S3 *p)
  return (S3 *) ((char *) p + fn4 (p->s9));

extern __attribute__((regparm (3))) int fn5 (void);

static inline __attribute__ ((always_inline)) int
fn6 (S3 *w, int x, S2 *y, S4 *z)
  int a = 0;
  char *b = (char *) w;
  S2 c = *y;

  while ((char *) w < b + x)
      if (w->s10 && w->s8)
          fn2 (w->s12, w->s10, &c);
          z->s4 = c.s4;
          z->s13 = (unsigned short) ((char *) w - b);
          z->s14 = w->s9;
          fn5 ();

      w = fn3 (w);
  return a;

__attribute__((regparm (3))) unsigned int
test (void *u, S6 *v, S1 **w, S7 *x, S2 *y, S1 *z)
  unsigned b = v->s17->s16;
  unsigned a;
  S4 *c;
  unsigned d, e, f, i;

  fn1 (__func__, u, x->s18);
  c = (S4 *) (z->s3 + b);
  a = fn6 ((S3 *) (*w)->s3, b, y, c);
  c -= a;
  f = 0;
  e = 0;
  for (i = a - 1; ; i--)
      if (f + (unsigned short) (c[i].s14 / 2) > b / 2)
      f += c[i].s14;
  d = a - e;
  return c[d].s4;

since the PR28690 backport.  The c[i].s14 read is done using
movl    (%ecx), %edi    # <variable>.s14, D.1321
rather than
movw (%ecx), %di
which is wrong in this case, as %ecx is provably not 32-bit aligned (%ecx - 6 is
known to be 32-bit aligned).  Shouldn't be hard to transform this into an executable testcase (put the array of S14 at the end of mmaped page such that
the last c[i].s14 is right before end of page).
This is a regression since 4.3.0.
Comment 1 Andrew Pinski 2008-06-13 21:46:34 UTC
aligned_operand looks wrong:
  /* Look for some component that isn't known to be aligned.  */
  if (parts.index)
      if (REGNO_POINTER_ALIGN (REGNO (parts.index)) * parts.scale < 32)
        return 0;
  if (parts.base)
      if (REGNO_POINTER_ALIGN (REGNO (parts.base)) < 32)
        return 0;

We are using the hard registers here so we can have the wrong alignment as they are shared.

Why are we looking into register pointer's alignment anyways?  The MEM_ALIGN check about should give the correct information anyways.
Comment 2 Peter Bergner 2008-06-13 22:14:40 UTC
We shouldn't be attempting to call mark_reg_pointer in set_reg_attrs_from_value for a hard reg, since they can be shared between different values.  Andrew mentioned we maybe shouldn't be calling set_reg_attrs_from_value() with a hard reg, or we could do the following:

--- emit-rtl.c	(revision 134986)
+++ emit-rtl.c	(working copy)
@@ -969,14 +969,14 @@ set_reg_attrs_from_value (rtx reg, rtx x
       if (MEM_OFFSET (x) && GET_CODE (MEM_OFFSET (x)) == CONST_INT)
 	REG_ATTRS (reg)
 	  = get_reg_attrs (MEM_EXPR (x), INTVAL (MEM_OFFSET (x)) + offset);
-      if (MEM_POINTER (x))
+      if (!HARD_REGISTER_P (reg) && MEM_POINTER (x))
 	mark_reg_pointer (reg, MEM_ALIGN (x));
   else if (REG_P (x))
       if (REG_ATTRS (x))
 	update_reg_offset (reg, x, offset);
-      if (REG_POINTER (x))
+      if (!HARD_REGISTER_P (reg) && REG_POINTER (x))
 	mark_reg_pointer (reg, REGNO_POINTER_ALIGN (REGNO (x)));
Comment 3 Jakub Jelinek 2008-06-13 22:19:55 UTC
In addition to that, I wonder whether i386's aligned_operand shouldn't
prefer ORIGINAL_REGNO over REGNO when checking pointer alignment.
Comment 4 Jakub Jelinek 2008-06-23 13:07:02 UTC
Subject: Bug 36533

Author: jakub
Date: Mon Jun 23 13:06:15 2008
New Revision: 137038

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=137038
	PR target/36533
	* emit-rtl.c (set_reg_attrs_from_value): Do nothing if
	REG is a hard register.

	* gcc.target/i386/pr36533.c: New test.


Comment 5 Jakub Jelinek 2008-06-23 13:16:56 UTC
Subject: Bug 36533

Author: jakub
Date: Mon Jun 23 13:16:07 2008
New Revision: 137039

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=137039
	PR target/36533
	* emit-rtl.c (set_reg_attrs_from_value): Do nothing if
	REG is a hard register.

	* gcc.target/i386/pr36533.c: New test.


Comment 6 Jakub Jelinek 2008-06-23 13:19:20 UTC