Patchwork [rs6000] Fix PR16458, eliminate redudant compares

login
register
mail settings
Submitter Peter Bergner
Date April 10, 2012, 5:36 p.m.
Message ID <1334079412.10401.14.camel@otta>
Download mbox | patch
Permalink /patch/151676/
State New
Headers show

Comments

Peter Bergner - April 10, 2012, 5:36 p.m.
On Mon, 2012-01-30 at 10:46 +0100, Richard Guenther wrote:
> On Fri, Jan 27, 2012 at 5:56 PM, Peter Bergner <bergner@vnet.ibm.com> wrote:
> > This patch fixes PR16458 by using the type expression attached to a reg
> > rtx to detect its signedness and generating unsigned compares when
> > appropriate.  However, we continue to use signed compares for the
> > special case of when we compare an unsigned reg against the constant 0.
> > This is because signed and unsigned compares give the same results
> > for equality and "(unsigned)x > 0)" is equivalent to "x != 0".
> > Using a signed compare in this special case allows us to continue to
> > generate record form instructions (ie, instructions that implicitly
> > set cr0).
[snip]
> 
> This does not sound suitable for stage4.  rs6000_unsigned_reg_p
> looks suspiciously non-rs6000 specific, and if we really can rely
> on the way it is implemented should find its way to general RTL
> helper routines.

Now that we're in stage1 again, I'm attaching an updated patch that moves
rs6000_unsigned_reg_p into an arch independent RTL file like you wanted.
I have also attached a couple of patch hunks from Michael that sets register
attributes in a couple of cases we didn't before and that allows my patch
to coalesce the compares in pr16458-4.c.  The description of that issue is
located in:

    http://gcc.gnu.org/ml/gcc/2012-01/msg00339.html


The updated and expanded patch passes bootstrap and regtesting with no
regessions on powerpc64-linux.  Ok for mainline now?


Peter


2012-mm-dd  Peter Bergner  <bergner@vnet.ibm.com>
            Michael Matz  <matz@suse.de>

gcc/
        PR target/16458
	* rtlanal.c (unsigned_reg_p): New function.
	* rtl.h (unsigned_reg_p): Prototype it.
	* config/rs6000/rs6000.c (rs6000_generate_compare): Use it.
	Update comment.
	* expr.c (expand_expr_real_1): Set register attributes.
	* stmt.c (expand_case): Likewise.

gcc/testsuite/
        PR target/16458
        * gcc.target/powerpc/pr16458-1.c: New test.
        * gcc.target/powerpc/pr16458-2.c: Likewise.
        * gcc.target/powerpc/pr16458-3.c: Likewise.
        * gcc.target/powerpc/pr16458-4.c: Likewise.
David Edelsohn - April 10, 2012, 6:50 p.m.
On Tue, Apr 10, 2012 at 1:36 PM, Peter Bergner <bergner@vnet.ibm.com> wrote:

> 2012-mm-dd  Peter Bergner  <bergner@vnet.ibm.com>
>            Michael Matz  <matz@suse.de>
>
> gcc/
>        PR target/16458
>        * rtlanal.c (unsigned_reg_p): New function.
>        * rtl.h (unsigned_reg_p): Prototype it.
>        * config/rs6000/rs6000.c (rs6000_generate_compare): Use it.
>        Update comment.
>        * expr.c (expand_expr_real_1): Set register attributes.
>        * stmt.c (expand_case): Likewise.
>
> gcc/testsuite/
>        PR target/16458
>        * gcc.target/powerpc/pr16458-1.c: New test.
>        * gcc.target/powerpc/pr16458-2.c: Likewise.
>        * gcc.target/powerpc/pr16458-3.c: Likewise.
>        * gcc.target/powerpc/pr16458-4.c: Likewise.

The rs6000 and testsuite parts of the patch are okay with me.

Thanks, David
Richard Guenther - April 11, 2012, 8:43 a.m.
On Tue, Apr 10, 2012 at 8:50 PM, David Edelsohn <dje.gcc@gmail.com> wrote:
> On Tue, Apr 10, 2012 at 1:36 PM, Peter Bergner <bergner@vnet.ibm.com> wrote:
>
>> 2012-mm-dd  Peter Bergner  <bergner@vnet.ibm.com>
>>            Michael Matz  <matz@suse.de>
>>
>> gcc/
>>        PR target/16458
>>        * rtlanal.c (unsigned_reg_p): New function.
>>        * rtl.h (unsigned_reg_p): Prototype it.
>>        * config/rs6000/rs6000.c (rs6000_generate_compare): Use it.
>>        Update comment.
>>        * expr.c (expand_expr_real_1): Set register attributes.
>>        * stmt.c (expand_case): Likewise.
>>
>> gcc/testsuite/
>>        PR target/16458
>>        * gcc.target/powerpc/pr16458-1.c: New test.
>>        * gcc.target/powerpc/pr16458-2.c: Likewise.
>>        * gcc.target/powerpc/pr16458-3.c: Likewise.
>>        * gcc.target/powerpc/pr16458-4.c: Likewise.
>
> The rs6000 and testsuite parts of the patch are okay with me.

The rest is ok as well.

Thanks,
Richard.

> Thanks, David
Peter Bergner - April 11, 2012, 11:54 a.m.
On Wed, 2012-04-11 at 10:43 +0200, Richard Guenther wrote:
> On Tue, Apr 10, 2012 at 8:50 PM, David Edelsohn <dje.gcc@gmail.com> wrote:
> > On Tue, Apr 10, 2012 at 1:36 PM, Peter Bergner <bergner@vnet.ibm.com> wrote:
> >
> >> 2012-mm-dd  Peter Bergner  <bergner@vnet.ibm.com>
> >>            Michael Matz  <matz@suse.de>
> >>
> >> gcc/
> >>        PR target/16458
> >>        * rtlanal.c (unsigned_reg_p): New function.
> >>        * rtl.h (unsigned_reg_p): Prototype it.
> >>        * config/rs6000/rs6000.c (rs6000_generate_compare): Use it.
> >>        Update comment.
> >>        * expr.c (expand_expr_real_1): Set register attributes.
> >>        * stmt.c (expand_case): Likewise.
> >>
> >> gcc/testsuite/
> >>        PR target/16458
> >>        * gcc.target/powerpc/pr16458-1.c: New test.
> >>        * gcc.target/powerpc/pr16458-2.c: Likewise.
> >>        * gcc.target/powerpc/pr16458-3.c: Likewise.
> >>        * gcc.target/powerpc/pr16458-4.c: Likewise.
> >
> > The rs6000 and testsuite parts of the patch are okay with me.
> 
> The rest is ok as well.

Thanks, I have committed this as revision 186312.

Peter

Patch

Index: gcc/rtlanal.c
===================================================================
--- gcc/rtlanal.c	(revision 186244)
+++ gcc/rtlanal.c	(working copy)
@@ -636,6 +636,25 @@  count_occurrences (const_rtx x, const_rt
 }
 
 
+/* Return TRUE if OP is a register or subreg of a register that
+   holds an unsigned quantity.  Otherwise, return FALSE.  */
+
+bool
+unsigned_reg_p (rtx op)
+{
+  if (REG_P (op)
+      && REG_EXPR (op)
+      && TYPE_UNSIGNED (TREE_TYPE (REG_EXPR (op))))
+    return true;
+
+  if (GET_CODE (op) == SUBREG
+      && SUBREG_PROMOTED_UNSIGNED_P (op))
+    return true;
+
+  return false;
+}
+
+
 /* Nonzero if register REG appears somewhere within IN.
    Also works if REG is not a register; in this case it checks
    for a subexpression of IN that is Lisp "equal" to REG.  */
Index: gcc/rtl.h
===================================================================
--- gcc/rtl.h	(revision 186244)
+++ gcc/rtl.h	(working copy)
@@ -1909,6 +1909,7 @@  extern HOST_WIDE_INT get_integer_term (c
 extern rtx get_related_value (const_rtx);
 extern bool offset_within_block_p (const_rtx, HOST_WIDE_INT);
 extern void split_const (rtx, rtx *, rtx *);
+extern bool unsigned_reg_p (rtx);
 extern int reg_mentioned_p (const_rtx, const_rtx);
 extern int count_occurrences (const_rtx, const_rtx, int);
 extern int reg_referenced_p (const_rtx, const_rtx);
Index: gcc/config/rs6000/rs6000.c
===================================================================
--- gcc/config/rs6000/rs6000.c	(revision 186244)
+++ gcc/config/rs6000/rs6000.c	(working copy)
@@ -15570,14 +15570,11 @@  rs6000_generate_compare (rtx cmp, enum m
 	   || code == GEU || code == LEU)
     comp_mode = CCUNSmode;
   else if ((code == EQ || code == NE)
-	   && GET_CODE (op0) == SUBREG
-	   && GET_CODE (op1) == SUBREG
-	   && SUBREG_PROMOTED_UNSIGNED_P (op0)
-	   && SUBREG_PROMOTED_UNSIGNED_P (op1))
+	   && unsigned_reg_p (op0)
+	   && (unsigned_reg_p (op1)
+	       || (CONST_INT_P (op1) && INTVAL (op1) != 0)))
     /* These are unsigned values, perhaps there will be a later
-       ordering compare that can be shared with this one.
-       Unfortunately we cannot detect the signedness of the operands
-       for non-subregs.  */
+       ordering compare that can be shared with this one.  */
     comp_mode = CCUNSmode;
   else
     comp_mode = CCmode;
Index: gcc/expr.c
===================================================================
--- gcc/expr.c	(revision 186244)
+++ gcc/expr.c	(working copy)
@@ -9015,8 +9015,13 @@  expand_expr_real_1 (tree exp, rtx target
 	  && stmt_is_replaceable_p (SSA_NAME_DEF_STMT (exp)))
 	g = SSA_NAME_DEF_STMT (exp);
       if (g)
-	return expand_expr_real (gimple_assign_rhs_to_tree (g), target, tmode,
-				 modifier, NULL);
+	{
+	  rtx r = expand_expr_real (gimple_assign_rhs_to_tree (g), target,
+				    tmode, modifier, NULL);
+	  if (REG_P (r) && !REG_EXPR (r))
+	    set_reg_attrs_for_decl_rtl (SSA_NAME_VAR (exp), r);
+	  return r;
+	}
 
       ssa_name = exp;
       decl_rtl = get_rtx_for_ssa_name (ssa_name);
Index: gcc/stmt.c
===================================================================
--- gcc/stmt.c	(revision 186244)
+++ gcc/stmt.c	(working copy)
@@ -2381,7 +2381,11 @@  expand_case (gimple stmt)
 	  do_pending_stack_adjust ();
 
 	  if (MEM_P (index))
-	    index = copy_to_reg (index);
+	    {
+	      index = copy_to_reg (index);
+	      if (TREE_CODE (index_expr) == SSA_NAME)
+		set_reg_attrs_for_decl_rtl (SSA_NAME_VAR (index_expr), index);
+	    }
 
 	  /* We generate a binary decision tree to select the
 	     appropriate target code.  This is done as follows:
Index: gcc/testsuite/gcc.target/powerpc/pr16458-1.c
===================================================================
--- gcc/testsuite/gcc.target/powerpc/pr16458-1.c	(revision 0)
+++ gcc/testsuite/gcc.target/powerpc/pr16458-1.c	(revision 0)
@@ -0,0 +1,18 @@ 
+/* Test cse'ing of unsigned compares.  */
+/* { dg-do compile } */
+/* { dg-options "-O2" } */
+
+/* { dg-final { scan-assembler-not "cmpw" } } */
+/* { dg-final { scan-assembler-times "cmplw" 1 } } */
+
+unsigned int a, b;
+
+int
+foo (void)
+{
+  if (a == b) return 1;
+  if (a > b)  return 2;
+  if (a < b)  return 3;
+  if (a != b) return 4;
+  return 0;
+}
Index: gcc/testsuite/gcc.target/powerpc/pr16458-2.c
===================================================================
--- gcc/testsuite/gcc.target/powerpc/pr16458-2.c	(revision 0)
+++ gcc/testsuite/gcc.target/powerpc/pr16458-2.c	(revision 0)
@@ -0,0 +1,18 @@ 
+/* Test cse'ing of unsigned compares.  */
+/* { dg-do compile } */
+/* { dg-options "-O2" } */
+
+/* { dg-final { scan-assembler-not "cmpw" } } */
+/* { dg-final { scan-assembler-times "cmplw" 1 } } */
+
+unsigned int *a, *b;
+
+int
+foo (void)
+{
+  if (*a == *b) return 1;
+  if (*a > *b)  return 2;
+  if (*a < *b)  return 3;
+  if (*a != *b) return 4;
+  return 0;
+}
Index: gcc/testsuite/gcc.target/powerpc/pr16458-3.c
===================================================================
--- gcc/testsuite/gcc.target/powerpc/pr16458-3.c	(revision 0)
+++ gcc/testsuite/gcc.target/powerpc/pr16458-3.c	(revision 0)
@@ -0,0 +1,41 @@ 
+/* Test cse'ing of unsigned compares.  */
+/* { dg-do compile } */
+/* { dg-options "-O2 -fno-jump-tables" } */
+
+/* { dg-final { scan-assembler-not "cmpwi" } } */
+/* { dg-final { scan-assembler-times "cmplwi" 5 } } */
+
+extern int case0 (void);
+extern int case1 (void);
+extern int case2 (void);
+extern int case3 (void);
+extern int case4 (void);
+
+enum CASE_VALUES
+{
+  CASE0 = 1,
+  CASE1,
+  CASE2,
+  CASE3,
+  CASE4
+};
+
+int
+foo (enum CASE_VALUES index)
+{
+  switch (index)
+    {
+    case CASE0:
+      return case0 ();
+    case CASE1:
+      return case1 ();
+    case CASE2:
+      return case2 ();
+    case CASE3:
+      return case3 ();
+    case CASE4:
+      return case4 ();
+    }
+
+  return 0;
+}
Index: gcc/testsuite/gcc.target/powerpc/pr16458-4.c
===================================================================
--- gcc/testsuite/gcc.target/powerpc/pr16458-4.c	(revision 0)
+++ gcc/testsuite/gcc.target/powerpc/pr16458-4.c	(revision 0)
@@ -0,0 +1,44 @@ 
+/* Test cse'ing of unsigned compares.  */
+/* { dg-do compile } */
+/* { dg-options "-O2 -fno-jump-tables" } */
+
+/* The following tests fail due to an issue in expand not
+   attaching an type expression information on *index's reg rtx.  */
+
+/* { dg-final { scan-assembler-not "cmpwi" } } */
+/* { dg-final { scan-assembler-times "cmplwi" 5 } } */
+
+extern int case0 (void);
+extern int case1 (void);
+extern int case2 (void);
+extern int case3 (void);
+extern int case4 (void);
+
+enum CASE_VALUES
+{
+  CASE0 = 1,
+  CASE1,
+  CASE2,
+  CASE3,
+  CASE4
+};
+
+int
+foo (enum CASE_VALUES *index)
+{
+  switch (*index)
+    {
+    case CASE0:
+      return case0 ();
+    case CASE1:
+      return case1 ();
+    case CASE2:
+      return case2 ();
+    case CASE3:
+      return case3 ();
+    case CASE4:
+      return case4 ();
+    }
+
+  return 0;
+}