[PATCH] PR35371 GCSE loses track of REG_POINTER attribute

[PATCH] PR35371 GCSE loses track of REG_POINTER attribute

[Peter Bergner]

While tracking down an unrelated performance problem, I noticed that
for the following simple loop, we generate indexed form stores and that
we get the ordering of the operands wrong which causes a slowdown on
POWER6 (see PR28690 for more details).  The culprit here is GCSE.
When GCSE creates a pseudo reg to copy a reaching expression into,
it fails to copy the REG_POINTER attribute over to the new pseudo.
This new pseudo is then analyzed by the new code we added to PR28690
and since there is no REG_POINTER attribute, we fail to swap the operands
into the correct order.  This patch makes sure we inherit the REG_POINTER
attribute.

Is this patch ok for mainline once bootstrap and regression testing have
completed?  Given PR28690 was a 4.3 regression, is this ok for 4.3.1 once
that is open for fixes?

Peter

	PR rtl-optimization/35371
        * gcse.c (gen_reaching_reg_rtx): New function.
	(pre_delete): Use it.
	(hoist_code): Likewise.
	(build_store_vectors): Likewise.
	(delete_store): Likewise.

Index: gcse.c
===================================================================
--- gcse.c	(revision 132568)
+++ gcse.c	(working copy)
@@ -818,6 +818,24 @@ gcse_main (rtx f ATTRIBUTE_UNUSED)
 \f
 /* Misc. utilities.  */
 
+/* Create a pseudo reg to copy the result of a reaching expression into.
+   Be careful to inherit the REG_POINTER attribute.  */
+
+static rtx
+gen_reaching_reg_rtx (rtx x)
+{
+  rtx reg;
+
+  gcc_assert (REG_P (x));
+
+  reg = gen_reg_rtx (GET_MODE (x));
+
+  if (REG_POINTER (x))
+    mark_reg_pointer (reg, REGNO_POINTER_ALIGN (REGNO (x)));
+
+  return reg;
+}
+
 /* Nonzero for each mode that supports (set (reg) (reg)).
    This is trivially true for integer and floating point values.
    It may or may not be true for condition codes.  */
@@ -4456,8 +4474,7 @@ pre_delete (void)
 		   expressions into.  Get the mode for the new pseudo from
 		   the mode of the original destination pseudo.  */
 		if (expr->reaching_reg == NULL)
-		  expr->reaching_reg
-		    = gen_reg_rtx (GET_MODE (SET_DEST (set)));
+		  expr->reaching_reg = gen_reaching_reg_rtx (SET_DEST (set));
 
 		gcse_emit_move_after (expr->reaching_reg, SET_DEST (set), insn);
 		delete_insn (insn);
@@ -4981,7 +4998,7 @@ hoist_code (void)
 			 from the mode of the original destination pseudo.  */
 		      if (expr->reaching_reg == NULL)
 			expr->reaching_reg
-			  = gen_reg_rtx (GET_MODE (SET_DEST (set)));
+			  = gen_reaching_reg_rtx (SET_DEST (set));
 
 		      gcse_emit_move_after (expr->reaching_reg, SET_DEST (set), insn);
 		      delete_insn (insn);
@@ -6114,7 +6131,7 @@ build_store_vectors (void)
 	     are any side effects.  */
 	  if (TEST_BIT (ae_gen[bb->index], ptr->index))
 	    {
-	      rtx r = gen_reg_rtx (GET_MODE (ptr->pattern));
+	      rtx r = gen_reaching_reg_rtx (ptr->pattern);
 	      if (dump_file)
 		fprintf (dump_file, "Removing redundant store:\n");
 	      replace_store_insn (r, XEXP (st, 0), bb, ptr);
@@ -6437,7 +6454,7 @@ delete_store (struct ls_expr * expr, bas
   rtx reg, i, del;
 
   if (expr->reaching_reg == NULL_RTX)
-    expr->reaching_reg = gen_reg_rtx (GET_MODE (expr->pattern));
+    expr->reaching_reg = gen_reaching_reg_rtx (expr->pattern);
 
   reg = expr->reaching_reg;
 

Re: [PATCH] PR35371 GCSE loses track of REG_POINTER attribute

[Peter Bergner]

On Mon, 2008-02-25 at 16:26 -0600, Peter Bergner wrote:
> While tracking down an unrelated performance problem, I noticed that
> for the following simple loop, we generate indexed form stores and that
> we get the ordering of the operands wrong which causes a slowdown on
> POWER6 (see PR28690 for more details).  The culprit here is GCSE.

Sorry, I forgot to show the loop.  It's posted in the PR though:

  http://gcc.gnu.org/bugzilla/show_bug.cgi?id=35371

Peter

Re: [PATCH,updated] PR35371 GCSE loses track of REG_POINTER attribute

[Peter Bergner]

It seems we're not always passed a reg rtx the comments would seem to
imply.  I've updated the patch to handle that.  Bootstrap and regression
testing is in progress again.

Is this patch ok for mainline once bootstrap and regression testing have
completed?  Given PR28690 was a 4.3 regression, is this ok for 4.3.1 once
that is open for fixes?

Peter

	PR rtl-optimization/35371
	* gcse.c (gen_reaching_reg_rtx): New function.
	(pre_delete): Use it.
	(hoist_code): Likewise.
	(build_store_vectors): Likewise.
	(delete_store): Likewise.

Index: gcse.c
===================================================================
--- gcse.c	(revision 132568)
+++ gcse.c	(working copy)
@@ -818,6 +818,22 @@ gcse_main (rtx f ATTRIBUTE_UNUSED)
 \f
 /* Misc. utilities.  */
 
+/* Create a pseudo reg to copy the result of a reaching expression into.
+   Be careful to inherit the REG_POINTER attribute.  */
+
+static rtx
+gen_reaching_reg_rtx (rtx x)
+{
+  rtx reg = gen_reg_rtx (GET_MODE (x));
+
+  if (REG_P (x) && REG_POINTER (x))
+    mark_reg_pointer (reg, REGNO_POINTER_ALIGN (REGNO (x)));
+  else if (MEM_P (x) && MEM_POINTER (x))
+    mark_reg_pointer (reg, MEM_ALIGN (x));
+
+  return reg;
+}
+
 /* Nonzero for each mode that supports (set (reg) (reg)).
    This is trivially true for integer and floating point values.
    It may or may not be true for condition codes.  */
@@ -4456,8 +4472,7 @@ pre_delete (void)
 		   expressions into.  Get the mode for the new pseudo from
 		   the mode of the original destination pseudo.  */
 		if (expr->reaching_reg == NULL)
-		  expr->reaching_reg
-		    = gen_reg_rtx (GET_MODE (SET_DEST (set)));
+		  expr->reaching_reg = gen_reaching_reg_rtx (SET_DEST (set));
 
 		gcse_emit_move_after (expr->reaching_reg, SET_DEST (set), insn);
 		delete_insn (insn);
@@ -4981,7 +4996,7 @@ hoist_code (void)
 			 from the mode of the original destination pseudo.  */
 		      if (expr->reaching_reg == NULL)
 			expr->reaching_reg
-			  = gen_reg_rtx (GET_MODE (SET_DEST (set)));
+			  = gen_reaching_reg_rtx (SET_DEST (set));
 
 		      gcse_emit_move_after (expr->reaching_reg, SET_DEST (set), insn);
 		      delete_insn (insn);
@@ -6114,7 +6129,7 @@ build_store_vectors (void)
 	     are any side effects.  */
 	  if (TEST_BIT (ae_gen[bb->index], ptr->index))
 	    {
-	      rtx r = gen_reg_rtx (GET_MODE (ptr->pattern));
+	      rtx r = gen_reaching_reg_rtx (ptr->pattern);
 	      if (dump_file)
 		fprintf (dump_file, "Removing redundant store:\n");
 	      replace_store_insn (r, XEXP (st, 0), bb, ptr);
@@ -6437,7 +6452,7 @@ delete_store (struct ls_expr * expr, bas
   rtx reg, i, del;
 
   if (expr->reaching_reg == NULL_RTX)
-    expr->reaching_reg = gen_reg_rtx (GET_MODE (expr->pattern));
+    expr->reaching_reg = gen_reaching_reg_rtx (expr->pattern);
 
   reg = expr->reaching_reg;
 

Re: [PATCH,withdrawn] PR35371 GCSE loses track of REG_POINTER attribute

[Peter Bergner]

On Mon, 2008-02-25 at 22:27 -0600, Peter Bergner wrote:
> It seems we're not always passed a reg rtx the comments would seem to
> imply.  I've updated the patch to handle that.  Bootstrap and regression
> testing is in progress again.

While this was bootstrapping, I started playing around with different
opt levels with the test case.  It seems if I use -fno-gcse, then we
again fail to order the operands correctly.  So it, looks like there's
another location that needs updating to copy the REG_POINTER attribute.

I'll post another patch when I've found it.

Peter

Re: [PATCH] PR35371 GCSE loses track of REG_POINTER attribute

[Richard Sandiford]

Peter Bergner <bergner@vnet.ibm.com> writes:
> Index: gcse.c
> ===================================================================
> --- gcse.c	(revision 132568)
> +++ gcse.c	(working copy)
> @@ -818,6 +818,24 @@ gcse_main (rtx f ATTRIBUTE_UNUSED)
>  \f
>  /* Misc. utilities.  */
>  
> +/* Create a pseudo reg to copy the result of a reaching expression into.
> +   Be careful to inherit the REG_POINTER attribute.  */
> +
> +static rtx
> +gen_reaching_reg_rtx (rtx x)
> +{
> +  rtx reg;
> +
> +  gcc_assert (REG_P (x));
> +
> +  reg = gen_reg_rtx (GET_MODE (x));
> +
> +  if (REG_POINTER (x))
> +    mark_reg_pointer (reg, REGNO_POINTER_ALIGN (REGNO (x)));
> +
> +  return reg;
> +}
> +

Minor suggestion, but maybe this could go in emit-rtl.c, under a more
generic name?  Given the performance impact of losing pointer info,
it would be nice to have a defined API for creating a register that's
like another.

Richard

Re: [PATCH] PR35371 GCSE loses track of REG_POINTER attribute

[Peter Bergner]

On Tue, 2008-02-26 at 17:29 +0000, Richard Sandiford wrote:
> Minor suggestion, but maybe this could go in emit-rtl.c, under a more
> generic name?  Given the performance impact of losing pointer info,
> it would be nice to have a defined API for creating a register that's
> like another.

Having tracked a similar problem in another file, I was thinking along
the same lines.  I'm bad at names though.  Care to suggest a name for
the new function?

Peter

Re: [PATCH] PR35371 GCSE loses track of REG_POINTER attribute

[Richard Sandiford]

Peter Bergner <bergner@vnet.ibm.com> writes:
> On Tue, 2008-02-26 at 17:29 +0000, Richard Sandiford wrote:
>> Minor suggestion, but maybe this could go in emit-rtl.c, under a more
>> generic name?  Given the performance impact of losing pointer info,
>> it would be nice to have a defined API for creating a register that's
>> like another.
>
> Having tracked a similar problem in another file, I was thinking along
> the same lines.  I'm bad at names though.  Care to suggest a name for
> the new function?

Me too, which is why I copped out. ;)  Oh well.  How about
gen_reg_rtx_like?

Richard

Re: [PATCH] PR35371 GCSE loses track of REG_POINTER attribute

[Jeff Law]

Peter Bergner wrote:
> On Tue, 2008-02-26 at 17:29 +0000, Richard Sandiford wrote:
>> Minor suggestion, but maybe this could go in emit-rtl.c, under a more
>> generic name?  Given the performance impact of losing pointer info,
>> it would be nice to have a defined API for creating a register that's
>> like another.
> 
> Having tracked a similar problem in another file, I was thinking along
> the same lines.  I'm bad at names though.  Care to suggest a name for
> the new function?
If someone wanted to get real ambitious they could revamp the
REG_POINTER propagation code as well.  It's amazingly simplistic
at the moment (see regclass.c:reg_scan_mark_refs).  Basically it
fails to propagate for any register destination that is set more
than once, even if all the sets are of the proper form for
propagating REG_POINTER.

Jeff

Re: [PATCH] PR35371 GCSE loses track of REG_POINTER attribute

[Peter Bergner]

[-- Attachment #1: Type: text/plain, Size: 1182 bytes --]

On Tue, 2008-02-26 at 12:25 -0700, Jeff Law wrote:
> If someone wanted to get real ambitious they could revamp the
> REG_POINTER propagation code as well.  It's amazingly simplistic
> at the moment (see regclass.c:reg_scan_mark_refs).  Basically it
> fails to propagate for any register destination that is set more
> than once, even if all the sets are of the proper form for
> propagating REG_POINTER.

Do you mean fix it up and then call it from more than just CSE?
Currently, the only call to reg_scan() isn't in a location that
will help me.

Anyway, I took Richard's advice and moved/renamed the new function
to emit-rtl.c.  How does the new code look?  For -O1 compiles, I had
to properly order the indexed load/store operands during expand, because
we never attempt to fix them up after that (actually, DSE seems to call
simplify, and swap_commutative_operands_p() correctly says we should
swap the operands, but for some reason I don't understand yet, DSE
seems to just throw away that result).


HJ,

Given the x86/x86_64 issues with the last indexed load/store patch,
can you SPEC test this patch to make sure the rtlanal.c change doesn't
affect you?  Thanks.


Peter




[-- Attachment #2: PR35371-3.diff --]
[-- Type: text/x-patch, Size: 6588 bytes --]

	PR rtl-optimization/35371
	* rtlanal.c: Update copyright year.
	(commutative_operand_precedence): Give SYMBOL_REF's the same
	precedence as REG_POINTER's and MEM_POINTER's.
	* emit-rtl.c: Update copyright year.
	(set_reg_attrs_from_value): Copy the REG_POINTER/MEM_POINTER
	attribute over to the new reg rtx.
	(gen_reg_rtx_copy): New function.
	* gcse.c: Update copyright year.
	(pre_delete): Call gen_reg_rtx_copy rather than gen_reg_rtx.
	(hoist_code): Likewise.
	(build_store_vectors): Likewise.
	(delete_store): Likewise.
	* loop-invariant.c: Update copyright year.
	(move_invariant_reg): Call gen_reg_rtx_copy rather than gen_reg_rtx.
	* rtl.h: Update copyright year.
	(gen_reg_rtx_copy): Add prototype.

Index: rtlanal.c
===================================================================
--- rtlanal.c	(revision 132568)
+++ rtlanal.c	(working copy)
@@ -1,6 +1,6 @@
 /* Analyze RTL for GNU compiler.
    Copyright (C) 1987, 1988, 1992, 1993, 1994, 1995, 1996, 1997, 1998,
-   1999, 2000, 2001, 2002, 2003, 2004, 2005, 2006, 2007 Free Software
+   1999, 2000, 2001, 2002, 2003, 2004, 2005, 2006, 2007, 2008 Free Software
    Foundation, Inc.
 
 This file is part of GCC.
@@ -2898,6 +2898,8 @@ commutative_operand_precedence (rtx op)
   switch (GET_RTX_CLASS (code))
     {
     case RTX_CONST_OBJ:
+      if (code == SYMBOL_REF)
+	return -1;
       if (code == CONST_INT)
         return -6;
       if (code == CONST_DOUBLE)
Index: gcse.c
===================================================================
--- gcse.c	(revision 132568)
+++ gcse.c	(working copy)
@@ -1,7 +1,7 @@
 /* Global common subexpression elimination/Partial redundancy elimination
    and global constant/copy propagation for GNU compiler.
    Copyright (C) 1997, 1998, 1999, 2000, 2001, 2002, 2003, 2004, 2005,
-   2006, 2007 Free Software Foundation, Inc.
+   2006, 2007, 2008 Free Software Foundation, Inc.
 
 This file is part of GCC.
 
@@ -4456,8 +4456,7 @@ pre_delete (void)
 		   expressions into.  Get the mode for the new pseudo from
 		   the mode of the original destination pseudo.  */
 		if (expr->reaching_reg == NULL)
-		  expr->reaching_reg
-		    = gen_reg_rtx (GET_MODE (SET_DEST (set)));
+		  expr->reaching_reg = gen_reg_rtx_copy (SET_DEST (set));
 
 		gcse_emit_move_after (expr->reaching_reg, SET_DEST (set), insn);
 		delete_insn (insn);
@@ -4981,7 +4980,7 @@ hoist_code (void)
 			 from the mode of the original destination pseudo.  */
 		      if (expr->reaching_reg == NULL)
 			expr->reaching_reg
-			  = gen_reg_rtx (GET_MODE (SET_DEST (set)));
+			  = gen_reg_rtx_copy (SET_DEST (set));
 
 		      gcse_emit_move_after (expr->reaching_reg, SET_DEST (set), insn);
 		      delete_insn (insn);
@@ -6114,7 +6113,7 @@ build_store_vectors (void)
 	     are any side effects.  */
 	  if (TEST_BIT (ae_gen[bb->index], ptr->index))
 	    {
-	      rtx r = gen_reg_rtx (GET_MODE (ptr->pattern));
+	      rtx r = gen_reg_rtx_copy (ptr->pattern);
 	      if (dump_file)
 		fprintf (dump_file, "Removing redundant store:\n");
 	      replace_store_insn (r, XEXP (st, 0), bb, ptr);
@@ -6437,7 +6436,7 @@ delete_store (struct ls_expr * expr, bas
   rtx reg, i, del;
 
   if (expr->reaching_reg == NULL_RTX)
-    expr->reaching_reg = gen_reg_rtx (GET_MODE (expr->pattern));
+    expr->reaching_reg = gen_reg_rtx_copy (expr->pattern);
 
   reg = expr->reaching_reg;
 
Index: emit-rtl.c
===================================================================
--- emit-rtl.c	(revision 132568)
+++ emit-rtl.c	(working copy)
@@ -1,6 +1,6 @@
 /* Emit RTL for the GCC expander.
    Copyright (C) 1987, 1988, 1992, 1993, 1994, 1995, 1996, 1997, 1998,
-   1999, 2000, 2001, 2002, 2003, 2004, 2005, 2006, 2007
+   1999, 2000, 2001, 2002, 2003, 2004, 2005, 2006, 2007, 2008
    Free Software Foundation, Inc.
 
 This file is part of GCC.
@@ -961,11 +961,32 @@ set_reg_attrs_from_value (rtx reg, rtx x
   int offset;
 
   offset = byte_lowpart_offset (GET_MODE (reg), GET_MODE (x));
-  if (MEM_P (x) && 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 (REG_P (x) && REG_ATTRS (x))
-    update_reg_offset (reg, x, offset);
+  if (MEM_P (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))
+	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))
+	mark_reg_pointer (reg, REGNO_POINTER_ALIGN (REGNO (x)));
+    }
+}
+
+/* Generate a REG rtx for a new pseudo register, copying the mode
+   and attributes from X.  */
+
+rtx
+gen_reg_rtx_copy (rtx x)
+{
+  rtx reg = gen_reg_rtx (GET_MODE (x));
+  set_reg_attrs_from_value (reg, x);
+  return reg;
 }
 
 /* Set the register attributes for registers contained in PARM_RTX.
Index: loop-invariant.c
===================================================================
--- loop-invariant.c	(revision 132568)
+++ loop-invariant.c	(working copy)
@@ -1,5 +1,5 @@
 /* RTL-level loop invariant motion.
-   Copyright (C) 2004, 2005, 2006, 2007 Free Software Foundation, Inc.
+   Copyright (C) 2004, 2005, 2006, 2007, 2008 Free Software Foundation, Inc.
 
 This file is part of GCC.
 
@@ -1193,7 +1193,7 @@ move_invariant_reg (struct loop *loop, u
 	 need to create a temporary register.  */
       set = single_set (inv->insn);
       dest = SET_DEST (set);
-      reg = gen_reg_rtx (GET_MODE (dest));
+      reg = gen_reg_rtx_copy (dest);
 
       /* Try replacing the destination by a new pseudoregister.  */
       if (!validate_change (inv->insn, &SET_DEST (set), reg, false))
Index: rtl.h
===================================================================
--- rtl.h	(revision 132568)
+++ rtl.h	(working copy)
@@ -1,6 +1,6 @@
 /* Register Transfer Language (RTL) definitions for GCC
    Copyright (C) 1987, 1991, 1992, 1993, 1994, 1995, 1996, 1997, 1998, 1999,
-   2000, 2001, 2002, 2003, 2004, 2005, 2006, 2007
+   2000, 2001, 2002, 2003, 2004, 2005, 2006, 2007, 2008
    Free Software Foundation, Inc.
 
 This file is part of GCC.
@@ -1509,6 +1509,7 @@ extern rtvec gen_rtvec_v (int, rtx *);
 extern rtx gen_reg_rtx (enum machine_mode);
 extern rtx gen_rtx_REG_offset (rtx, enum machine_mode, unsigned int, int);
 extern rtx gen_reg_rtx_offset (rtx, enum machine_mode, int);
+extern rtx gen_reg_rtx_copy (rtx);
 extern rtx gen_label_rtx (void);
 extern rtx gen_lowpart_common (enum machine_mode, rtx);
 

Re: [PATCH] PR35371 GCSE loses track of REG_POINTER attribute

[Jeff Law]

Peter Bergner wrote:
> On Tue, 2008-02-26 at 12:25 -0700, Jeff Law wrote:
>> If someone wanted to get real ambitious they could revamp the
>> REG_POINTER propagation code as well.  It's amazingly simplistic
>> at the moment (see regclass.c:reg_scan_mark_refs).  Basically it
>> fails to propagate for any register destination that is set more
>> than once, even if all the sets are of the proper form for
>> propagating REG_POINTER.
> 
> Do you mean fix it up and then call it from more than just CSE?
> Currently, the only call to reg_scan() isn't in a location that
> will help me.
No.  I mean make it smarter.  If you read the code it's amazingly
simplistic and punts propagation of REG_POINTER for any pseudo
that is set more than once.

It shouldn't be terribly difficult to build a simple propagation
engine that handles multiple sets.


Jeff

Re: [PATCH] PR35371 GCSE loses track of REG_POINTER attribute

[Peter Bergner]

On Mon, 2008-03-03 at 12:15 -0700, Jeff Law wrote:
> > Do you mean fix it up and then call it from more than just CSE?
> > Currently, the only call to reg_scan() isn't in a location that
> > will help me.
> No.  I mean make it smarter.  If you read the code it's amazingly
> simplistic and punts propagation of REG_POINTER for any pseudo
> that is set more than once.
> 
> It shouldn't be terribly difficult to build a simple propagation
> engine that handles multiple sets.

Sorry, making it "smarter" is what I meant by "fix it up".
My problem with it, as I mentioned in my previous note, is that
the only location it is currently called doesn't help me.
I guess what I was asking was there shouldn't be a problem
with me calling it from another location, correct?

H.J.,

Did you see my request to test the patch attached to:

  http://gcc.gnu.org/ml/gcc-patches/2008-02/msg01442.html

to verify I haven't changed x86/x86_64's SPEC scores due to
the rtlanal.c change?


Peter

Re: [PATCH] PR35371 GCSE loses track of REG_POINTER attribute

[Jeff Law]

Peter Bergner wrote:
> On Mon, 2008-03-03 at 12:15 -0700, Jeff Law wrote:
>>> Do you mean fix it up and then call it from more than just CSE?
>>> Currently, the only call to reg_scan() isn't in a location that
>>> will help me.
>> No.  I mean make it smarter.  If you read the code it's amazingly
>> simplistic and punts propagation of REG_POINTER for any pseudo
>> that is set more than once.
>>
>> It shouldn't be terribly difficult to build a simple propagation
>> engine that handles multiple sets.
> 
> Sorry, making it "smarter" is what I meant by "fix it up".
> My problem with it, as I mentioned in my previous note, is that
> the only location it is currently called doesn't help me.
> I guess what I was asking was there shouldn't be a problem
> with me calling it from another location, correct?
Ah.  A misunderstanding on my part.


I do recall some problems with passes substituting a pseudo
without REG_POINTER set for a register with REG_POINTER set,
so if you run it early, you might lose some REG_POINTER
attributes.

That's not a fatal problem -- the PA backend already knows
to cope with this issue.  And if the pass is safe, you ought
to be able to just run it twice.


Ideally propagation of REG_POINTER would use both the
assignment to the pseudo and the use of the pseudo to
try and determine if it's a pointer.


Jeff

[PING H.J. Lu] Re: [PATCH] PR35371 GCSE loses track of REG_POINTER attribute

[Peter Bergner]

On Thu, 2008-02-28 at 17:08 -0600, Peter Bergner wrote:
> HJ,
> 
> Given the x86/x86_64 issues with the last indexed load/store patch,
> can you SPEC test this patch to make sure the rtlanal.c change doesn't
> affect you?  Thanks.

HJ,

If you get a chance, can you please SPEC test the patch located in:

    http://gcc.gnu.org/ml/gcc-patches/2008-02/msg01442.html

just to make sure it doesn't have a negative impact on x86/x86_64?
Thanks.

Peter

Re: [PING H.J. Lu] Re: [PATCH] PR35371 GCSE loses track of REG_POINTER attribute

[H.J. Lu]

Hi Peter,

My email address has been changed to hjl.tools@gmail.com.

The current g++ is broken:

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=35494

It won't compile SPEC CPU 2006 correctly. I will test it after g++
is fixed. It will take about a week for SPEC CPU 2000/2006
runs.


H.J.
On Mon, Mar 10, 2008 at 8:32 AM, Peter Bergner <bergner@vnet.ibm.com> wrote:
> On Thu, 2008-02-28 at 17:08 -0600, Peter Bergner wrote:
>  > HJ,
>  >
>  > Given the x86/x86_64 issues with the last indexed load/store patch,
>  > can you SPEC test this patch to make sure the rtlanal.c change doesn't
>  > affect you?  Thanks.
>
>  HJ,
>
>  If you get a chance, can you please SPEC test the patch located in:
>
>
>     http://gcc.gnu.org/ml/gcc-patches/2008-02/msg01442.html
>
>  just to make sure it doesn't have a negative impact on x86/x86_64?
>  Thanks.
>
>  Peter
>
>
>

Re: [PING H.J. Lu] Re: [PATCH] PR35371 GCSE loses track of REG_POINTER attribute

[H.J. Lu]

On Mon, Mar 10, 2008 at 10:32:13AM -0500, Peter Bergner wrote:
> On Thu, 2008-02-28 at 17:08 -0600, Peter Bergner wrote:
> > HJ,
> > 
> > Given the x86/x86_64 issues with the last indexed load/store patch,
> > can you SPEC test this patch to make sure the rtlanal.c change doesn't
> > affect you?  Thanks.
> 
> HJ,
> 
> If you get a chance, can you please SPEC test the patch located in:
> 
>     http://gcc.gnu.org/ml/gcc-patches/2008-02/msg01442.html
> 
> just to make sure it doesn't have a negative impact on x86/x86_64?
> Thanks.

Hi Peter,

I can use gcc to compile SPEC CPU now. But your patch won't apply

patching file rtlanal.c
patching file gcse.c
Hunk #2 succeeded at 4463 (offset 7 lines).
Hunk #4 succeeded at 6120 (offset 7 lines).
patching file emit-rtl.c
Reversed (or previously applied) patch detected!  Assume -R? [n] 

against revision 133140. Do you have an updated patch?

Thanks.

H.J.

Re: [PING H.J. Lu] Re: [PATCH] PR35371 GCSE loses track of REG_POINTER attribute

[H.J. Lu]

[-- Attachment #1: Type: text/plain, Size: 1264 bytes --]

Hi Peter,

I removed copyright update in your original patch and applied it on
gcc 4.4 at revision 133082.  I didn't noticed any serious performance
regressions with SPEC CPU 2000/2006.

Thanks.

H.J.
On Wed, Mar 12, 2008 at 7:46 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
>
> On Mon, Mar 10, 2008 at 10:32:13AM -0500, Peter Bergner wrote:
>  > On Thu, 2008-02-28 at 17:08 -0600, Peter Bergner wrote:
>  > > HJ,
>  > >
>  > > Given the x86/x86_64 issues with the last indexed load/store patch,
>  > > can you SPEC test this patch to make sure the rtlanal.c change doesn't
>  > > affect you?  Thanks.
>  >
>  > HJ,
>  >
>  > If you get a chance, can you please SPEC test the patch located in:
>  >
>  >     http://gcc.gnu.org/ml/gcc-patches/2008-02/msg01442.html
>  >
>  > just to make sure it doesn't have a negative impact on x86/x86_64?
>  > Thanks.
>
>  Hi Peter,
>
>  I can use gcc to compile SPEC CPU now. But your patch won't apply
>
>  patching file rtlanal.c
>  patching file gcse.c
>  Hunk #2 succeeded at 4463 (offset 7 lines).
>  Hunk #4 succeeded at 6120 (offset 7 lines).
>  patching file emit-rtl.c
>  Reversed (or previously applied) patch detected!  Assume -R? [n]
>
>  against revision 133140. Do you have an updated patch?
>
>  Thanks.
>
>  H.J.
>

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: gcc-pr35371-2.patch --]
[-- Type: text/x-patch; name=gcc-pr35371-2.patch, Size: 6443 bytes --]

On Tue, 2008-02-26 at 12:25 -0700, Jeff Law wrote:
> If someone wanted to get real ambitious they could revamp the
> REG_POINTER propagation code as well.  It's amazingly simplistic
> at the moment (see regclass.c:reg_scan_mark_refs).  Basically it
> fails to propagate for any register destination that is set more
> than once, even if all the sets are of the proper form for
> propagating REG_POINTER.

Do you mean fix it up and then call it from more than just CSE?
Currently, the only call to reg_scan() isn't in a location that
will help me.

Anyway, I took Richard's advice and moved/renamed the new function
to emit-rtl.c.  How does the new code look?  For -O1 compiles, I had
to properly order the indexed load/store operands during expand, because
we never attempt to fix them up after that (actually, DSE seems to call
simplify, and swap_commutative_operands_p() correctly says we should
swap the operands, but for some reason I don't understand yet, DSE
seems to just throw away that result).


HJ,

Given the x86/x86_64 issues with the last indexed load/store patch,
can you SPEC test this patch to make sure the rtlanal.c change doesn't
affect you?  Thanks.


Peter




--=-PIIVCxfVeqdczuah+k/I
Content-Disposition: attachment; filename=PR35371-3.diff
Content-Type: text/x-patch; name=PR35371-3.diff; charset=UTF-8
Content-Transfer-Encoding: 7bit

	PR rtl-optimization/35371
	* rtlanal.c: Update copyright year.
	(commutative_operand_precedence): Give SYMBOL_REF's the same
	precedence as REG_POINTER's and MEM_POINTER's.
	* emit-rtl.c: Update copyright year.
	(set_reg_attrs_from_value): Copy the REG_POINTER/MEM_POINTER
	attribute over to the new reg rtx.
	(gen_reg_rtx_copy): New function.
	* gcse.c: Update copyright year.
	(pre_delete): Call gen_reg_rtx_copy rather than gen_reg_rtx.
	(hoist_code): Likewise.
	(build_store_vectors): Likewise.
	(delete_store): Likewise.
	* loop-invariant.c: Update copyright year.
	(move_invariant_reg): Call gen_reg_rtx_copy rather than gen_reg_rtx.
	* rtl.h: Update copyright year.
	(gen_reg_rtx_copy): Add prototype.

Index: rtlanal.c
===================================================================
--- rtlanal.c	(revision 132568)
+++ rtlanal.c	(working copy)
@@ -2898,6 +2898,8 @@ commutative_operand_precedence (rtx op)
   switch (GET_RTX_CLASS (code))
     {
     case RTX_CONST_OBJ:
+      if (code == SYMBOL_REF)
+	return -1;
       if (code == CONST_INT)
         return -6;
       if (code == CONST_DOUBLE)
Index: gcse.c
===================================================================
--- gcse.c	(revision 132568)
+++ gcse.c	(working copy)
@@ -4456,8 +4456,7 @@ pre_delete (void)
 		   expressions into.  Get the mode for the new pseudo from
 		   the mode of the original destination pseudo.  */
 		if (expr->reaching_reg == NULL)
-		  expr->reaching_reg
-		    = gen_reg_rtx (GET_MODE (SET_DEST (set)));
+		  expr->reaching_reg = gen_reg_rtx_copy (SET_DEST (set));
 
 		gcse_emit_move_after (expr->reaching_reg, SET_DEST (set), insn);
 		delete_insn (insn);
@@ -4981,7 +4980,7 @@ hoist_code (void)
 			 from the mode of the original destination pseudo.  */
 		      if (expr->reaching_reg == NULL)
 			expr->reaching_reg
-			  = gen_reg_rtx (GET_MODE (SET_DEST (set)));
+			  = gen_reg_rtx_copy (SET_DEST (set));
 
 		      gcse_emit_move_after (expr->reaching_reg, SET_DEST (set), insn);
 		      delete_insn (insn);
@@ -6114,7 +6113,7 @@ build_store_vectors (void)
 	     are any side effects.  */
 	  if (TEST_BIT (ae_gen[bb->index], ptr->index))
 	    {
-	      rtx r = gen_reg_rtx (GET_MODE (ptr->pattern));
+	      rtx r = gen_reg_rtx_copy (ptr->pattern);
 	      if (dump_file)
 		fprintf (dump_file, "Removing redundant store:\n");
 	      replace_store_insn (r, XEXP (st, 0), bb, ptr);
@@ -6437,7 +6436,7 @@ delete_store (struct ls_expr * expr, bas
   rtx reg, i, del;
 
   if (expr->reaching_reg == NULL_RTX)
-    expr->reaching_reg = gen_reg_rtx (GET_MODE (expr->pattern));
+    expr->reaching_reg = gen_reg_rtx_copy (expr->pattern);
 
   reg = expr->reaching_reg;
 
Index: emit-rtl.c
===================================================================
--- emit-rtl.c	(revision 132568)
+++ emit-rtl.c	(working copy)
@@ -961,11 +961,32 @@ set_reg_attrs_from_value (rtx reg, rtx x
   int offset;
 
   offset = byte_lowpart_offset (GET_MODE (reg), GET_MODE (x));
-  if (MEM_P (x) && 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 (REG_P (x) && REG_ATTRS (x))
-    update_reg_offset (reg, x, offset);
+  if (MEM_P (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))
+	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))
+	mark_reg_pointer (reg, REGNO_POINTER_ALIGN (REGNO (x)));
+    }
+}
+
+/* Generate a REG rtx for a new pseudo register, copying the mode
+   and attributes from X.  */
+
+rtx
+gen_reg_rtx_copy (rtx x)
+{
+  rtx reg = gen_reg_rtx (GET_MODE (x));
+  set_reg_attrs_from_value (reg, x);
+  return reg;
 }
 
 /* Set the register attributes for registers contained in PARM_RTX.
Index: loop-invariant.c
===================================================================
--- loop-invariant.c	(revision 132568)
+++ loop-invariant.c	(working copy)
@@ -1193,7 +1193,7 @@ move_invariant_reg (struct loop *loop, u
 	 need to create a temporary register.  */
       set = single_set (inv->insn);
       dest = SET_DEST (set);
-      reg = gen_reg_rtx (GET_MODE (dest));
+      reg = gen_reg_rtx_copy (dest);
 
       /* Try replacing the destination by a new pseudoregister.  */
       if (!validate_change (inv->insn, &SET_DEST (set), reg, false))
Index: rtl.h
===================================================================
--- rtl.h	(revision 132568)
+++ rtl.h	(working copy)
@@ -1509,6 +1509,7 @@ extern rtvec gen_rtvec_v (int, rtx *);
 extern rtx gen_reg_rtx (enum machine_mode);
 extern rtx gen_rtx_REG_offset (rtx, enum machine_mode, unsigned int, int);
 extern rtx gen_reg_rtx_offset (rtx, enum machine_mode, int);
+extern rtx gen_reg_rtx_copy (rtx);
 extern rtx gen_label_rtx (void);
 extern rtx gen_lowpart_common (enum machine_mode, rtx);
 

--=-PIIVCxfVeqdczuah+k/I--