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

["H.J. Lu" <hjl dot tools at gmail dot com>]

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.
>

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--