Re: patch: CSE bug when narrowing constants

[Eric Botcazou <ebotcazou at adacore dot com>]

> Although this bug hasn't been confirmed yet:
>   http://gcc.gnu.org/ml/gcc/2008-11/msg00360.html
> this patch fixes the problem:

Thanks.  While this patch is correct for little-endian, it is not for 
big-endian since the initial transformation applied on x is not necessarily 
the reverse of taking the low part.  For example, on big-endian,

  (subreg:HI (reg:DI 59) 0) -> (subreg:SI (reg:DI 59) 0)

is not such an extension since the HImode low part of the latter is

  (subreg:HI (reg:DI 59) 2)


It turns out that, because of the way lookup_as_function is invoked, this 
initial transformation can only be triggered for SUBREGs; not sure whether 
this was intended but that has been so since the code was added in 1998.
In fact one can seriously wonder because doing a widening PUT_MODE on a
SUBREG is a no-no on big-endian; for example

  (subreg:HI (reg:DI 59) 2) -> (subreg:SI (reg:DI 59) 2)

doesn't make much sense.  Of course the modified SUBREG is only used for a 
lookup so, in practice, nothing happens in almost all cases on big-endian.  
And, even on little-endian, you need a precise set of circumstances to see 
something changed; that's why the issue has been latent for so long.

Given the above investigation, I was tempted to just wipe out the problematic 
code in lookup_as_function.  However it can really help in corner cases, when 
fixed, for example on your code, so I re-implemented it only for SUBREGS.

Tested on i586-suse-linux and sparc-sun-solaris2.10, applied to all active 
branches.


2008-12-04  Eric Botcazou  <ebotcazou@adacore.com>
            Gary Funck  <gary@intrepid.com>

        * cse.c (lookup_as_function): Delete mode frobbing code.
	(equiv_constant): Re-implement it there for SUBREGs.


2008-12-04  Eric Botcazou  <ebotcazou@adacore.com>
            Gary Funck  <gary@intrepid.com>

        * cse.c (lookup_as_function): Delete mode frobbing code.
	(fold_rtx_subreg): Re-implement it there for SUBREGs.



2008-12-04  Eric Botcazou  <ebotcazou@adacore.com>

        * gcc.dg/union-5.c: New test.



-- 
Eric Botcazou

Index: cse.c
===================================================================
--- cse.c	(revision 142432)
+++ cse.c	(working copy)
@@ -1364,17 +1364,6 @@ lookup_as_function (rtx x, enum rtx_code
   struct table_elt *p
     = lookup (x, SAFE_HASH (x, VOIDmode), GET_MODE (x));
 
-  /* If we are looking for a CONST_INT, the mode doesn't really matter, as
-     long as we are narrowing.  So if we looked in vain for a mode narrower
-     than word_mode before, look for word_mode now.  */
-  if (p == 0 && code == CONST_INT
-      && GET_MODE_SIZE (GET_MODE (x)) < GET_MODE_SIZE (word_mode))
-    {
-      x = copy_rtx (x);
-      PUT_MODE (x, word_mode);
-      p = lookup (x, SAFE_HASH (x, VOIDmode), word_mode);
-    }
-
   if (p == 0)
     return 0;
 
@@ -3641,6 +3630,8 @@ equiv_constant (rtx x)
 
   if (GET_CODE (x) == SUBREG)
     {
+      enum machine_mode mode = GET_MODE (x);
+      enum machine_mode imode = GET_MODE (SUBREG_REG (x));
       rtx new_rtx;
 
       /* See if we previously assigned a constant value to this SUBREG.  */
@@ -3649,10 +3640,25 @@ equiv_constant (rtx x)
           || (new_rtx = lookup_as_function (x, CONST_FIXED)) != 0)
         return new_rtx;
 
+      /* If we didn't and if doing so makes sense, see if we previously
+	 assigned a constant value to the enclosing word mode SUBREG.  */
+      if (GET_MODE_SIZE (mode) < GET_MODE_SIZE (word_mode)
+	  && GET_MODE_SIZE (word_mode) < GET_MODE_SIZE (imode))
+	{
+	  int byte = SUBREG_BYTE (x) - subreg_lowpart_offset (mode, word_mode);
+	  if (byte >= 0 && (byte % UNITS_PER_WORD) == 0)
+	    {
+	      rtx y = gen_rtx_SUBREG (word_mode, SUBREG_REG (x), byte);
+	      new_rtx = lookup_as_function (y, CONST_INT);
+	      if (new_rtx)
+		return gen_lowpart (mode, new_rtx);
+	    }
+	}
+
+      /* Otherwise see if we already have a constant for the inner REG.  */
       if (REG_P (SUBREG_REG (x))
 	  && (new_rtx = equiv_constant (SUBREG_REG (x))) != 0)
-        return simplify_subreg (GET_MODE (x), new_rtx,
-				GET_MODE (SUBREG_REG (x)), SUBREG_BYTE (x));
+        return simplify_subreg (mode, new_rtx, imode, SUBREG_BYTE (x));
 
       return 0;
     }

Index: cse.c
===================================================================
--- cse.c	(revision 142433)
+++ cse.c	(working copy)
@@ -1363,17 +1363,6 @@ lookup_as_function (rtx x, enum rtx_code
   struct table_elt *p
     = lookup (x, SAFE_HASH (x, VOIDmode), GET_MODE (x));
 
-  /* If we are looking for a CONST_INT, the mode doesn't really matter, as
-     long as we are narrowing.  So if we looked in vain for a mode narrower
-     than word_mode before, look for word_mode now.  */
-  if (p == 0 && code == CONST_INT
-      && GET_MODE_SIZE (GET_MODE (x)) < GET_MODE_SIZE (word_mode))
-    {
-      x = copy_rtx (x);
-      PUT_MODE (x, word_mode);
-      p = lookup (x, SAFE_HASH (x, VOIDmode), word_mode);
-    }
-
   if (p == 0)
     return 0;
 
@@ -3626,6 +3615,8 @@ equiv_constant (rtx x)
 
   if (GET_CODE (x) == SUBREG)
     {
+      enum machine_mode mode = GET_MODE (x);
+      enum machine_mode imode = GET_MODE (SUBREG_REG (x));
       rtx new;
 
       /* See if we previously assigned a constant value to this SUBREG.  */
@@ -3634,10 +3625,25 @@ equiv_constant (rtx x)
           || (new = lookup_as_function (x, CONST_FIXED)) != 0)
         return new;
 
+      /* If we didn't and if doing so makes sense, see if we previously
+	 assigned a constant value to the enclosing word mode SUBREG.  */
+      if (GET_MODE_SIZE (mode) < GET_MODE_SIZE (word_mode)
+	  && GET_MODE_SIZE (word_mode) < GET_MODE_SIZE (imode))
+	{
+	  int byte = SUBREG_BYTE (x) - subreg_lowpart_offset (mode, word_mode);
+	  if (byte >= 0 && (byte % UNITS_PER_WORD) == 0)
+	    {
+	      rtx y = gen_rtx_SUBREG (word_mode, SUBREG_REG (x), byte);
+	      new = lookup_as_function (y, CONST_INT);
+	      if (new)
+		return gen_lowpart (mode, new);
+	    }
+	}
+
+      /* Otherwise see if we already have a constant for the inner REG.  */
       if (REG_P (SUBREG_REG (x))
 	  && (new = equiv_constant (SUBREG_REG (x))) != 0)
-        return simplify_subreg (GET_MODE (x), new,
-				GET_MODE (SUBREG_REG (x)), SUBREG_BYTE (x));
+        return simplify_subreg (mode, new, imode, SUBREG_BYTE (x));
 
       return 0;
     }

Index: cse.c
===================================================================
--- cse.c	(revision 140730)
+++ cse.c	(working copy)
@@ -1455,17 +1455,6 @@ lookup_as_function (rtx x, enum rtx_code
   struct table_elt *p
     = lookup (x, SAFE_HASH (x, VOIDmode), GET_MODE (x));
 
-  /* If we are looking for a CONST_INT, the mode doesn't really matter, as
-     long as we are narrowing.  So if we looked in vain for a mode narrower
-     than word_mode before, look for word_mode now.  */
-  if (p == 0 && code == CONST_INT
-      && GET_MODE_SIZE (GET_MODE (x)) < GET_MODE_SIZE (word_mode))
-    {
-      x = copy_rtx (x);
-      PUT_MODE (x, word_mode);
-      p = lookup (x, SAFE_HASH (x, VOIDmode), word_mode);
-    }
-
   if (p == 0)
     return 0;
 
@@ -3258,6 +3247,7 @@ static rtx
 fold_rtx_subreg (rtx x, rtx insn)
 {
   enum machine_mode mode = GET_MODE (x);
+  enum machine_mode imode = GET_MODE (SUBREG_REG (x));
   rtx folded_arg0;
   rtx const_arg0;
   rtx new;
@@ -3267,6 +3257,21 @@ fold_rtx_subreg (rtx x, rtx insn)
       || (new = lookup_as_function (x, CONST_DOUBLE)) != 0)
     return new;
 
+  /* If we didn't and if doing so makes sense, see if we previously
+     assigned a constant value to the enclosing word mode SUBREG.  */
+  if (GET_MODE_SIZE (mode) < GET_MODE_SIZE (word_mode)
+      && GET_MODE_SIZE (word_mode) < GET_MODE_SIZE (imode))
+    {
+      int byte = SUBREG_BYTE (x) - subreg_lowpart_offset (mode, word_mode);
+      if (byte >= 0 && (byte % UNITS_PER_WORD) == 0)
+	{
+	  rtx y = gen_rtx_SUBREG (word_mode, SUBREG_REG (x), byte);
+	  new = lookup_as_function (y, CONST_INT);
+	  if (new)
+	    return gen_lowpart (mode, new);
+	}
+    }
+
   /* If this is a paradoxical SUBREG, we have no idea what value the
      extra bits would have.  However, if the operand is equivalent to
      a SUBREG whose operand is the same as our mode, and all the modes
@@ -3275,9 +3280,8 @@ fold_rtx_subreg (rtx x, rtx insn)
 
      Similarly if we find an integer constant.  */
 
-  if (GET_MODE_SIZE (mode) > GET_MODE_SIZE (GET_MODE (SUBREG_REG (x))))
+  if (GET_MODE_SIZE (mode) > GET_MODE_SIZE (imode))
     {
-      enum machine_mode imode = GET_MODE (SUBREG_REG (x));
       struct table_elt *elt;
 
       if (GET_MODE_SIZE (mode) <= UNITS_PER_WORD
@@ -3310,8 +3314,7 @@ fold_rtx_subreg (rtx x, rtx insn)
 
   if (folded_arg0 != SUBREG_REG (x))
     {
-      new = simplify_subreg (mode, folded_arg0,
-			     GET_MODE (SUBREG_REG (x)), SUBREG_BYTE (x));
+      new = simplify_subreg (mode, folded_arg0, imode, SUBREG_BYTE (x));
       if (new)
 	return new;
     }

/* { dg-do run } */
/* { dg-options "-O -fgcse" } */

extern void abort(void);

typedef unsigned short int uint16_t;
typedef unsigned int uint32_t;
typedef unsigned long long uint64_t;

typedef struct
{
  uint16_t thread;
  uint16_t phase;
} s32;

typedef union
{
  uint32_t i;
  s32 s;
} u32;

typedef union
{
  uint64_t i;
  u32 u;
} u64;

static __attribute__((noinline))
void foo(int val)
{
  u64 data;
  uint32_t thread;

  data.u.i = 0x10000L;
  thread = data.u.s.thread;
  if (val)
    abort ();
  if (thread)
    abort ();
}

int main(void)
{
  foo (0);
  return 0;
}