* [PATCH] PR35371 GCSE loses track of REG_POINTER attribute @ 2008-02-25 22:30 Peter Bergner 2008-02-25 23:13 ` Peter Bergner ` (2 more replies) 0 siblings, 3 replies; 18+ messages in thread From: Peter Bergner @ 2008-02-25 22:30 UTC (permalink / raw) To: gcc-patches 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; ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] PR35371 GCSE loses track of REG_POINTER attribute 2008-02-25 22:30 [PATCH] PR35371 GCSE loses track of REG_POINTER attribute Peter Bergner @ 2008-02-25 23:13 ` Peter Bergner 2008-02-26 5:25 ` [PATCH,updated] " Peter Bergner 2008-02-26 18:00 ` [PATCH] " Richard Sandiford 2 siblings, 0 replies; 18+ messages in thread From: Peter Bergner @ 2008-02-25 23:13 UTC (permalink / raw) To: gcc-patches 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH,updated] PR35371 GCSE loses track of REG_POINTER attribute 2008-02-25 22:30 [PATCH] PR35371 GCSE loses track of REG_POINTER attribute Peter Bergner 2008-02-25 23:13 ` Peter Bergner @ 2008-02-26 5:25 ` Peter Bergner 2008-02-26 5:49 ` [PATCH,withdrawn] " Peter Bergner 2008-02-26 18:00 ` [PATCH] " Richard Sandiford 2 siblings, 1 reply; 18+ messages in thread From: Peter Bergner @ 2008-02-26 5:25 UTC (permalink / raw) To: gcc-patches 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; ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH,withdrawn] PR35371 GCSE loses track of REG_POINTER attribute 2008-02-26 5:25 ` [PATCH,updated] " Peter Bergner @ 2008-02-26 5:49 ` Peter Bergner 0 siblings, 0 replies; 18+ messages in thread From: Peter Bergner @ 2008-02-26 5:49 UTC (permalink / raw) To: gcc-patches 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] PR35371 GCSE loses track of REG_POINTER attribute 2008-02-25 22:30 [PATCH] PR35371 GCSE loses track of REG_POINTER attribute Peter Bergner 2008-02-25 23:13 ` Peter Bergner 2008-02-26 5:25 ` [PATCH,updated] " Peter Bergner @ 2008-02-26 18:00 ` Richard Sandiford 2008-02-26 19:04 ` Peter Bergner 2 siblings, 1 reply; 18+ messages in thread From: Richard Sandiford @ 2008-02-26 18:00 UTC (permalink / raw) To: Peter Bergner; +Cc: gcc-patches 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] PR35371 GCSE loses track of REG_POINTER attribute 2008-02-26 18:00 ` [PATCH] " Richard Sandiford @ 2008-02-26 19:04 ` Peter Bergner 2008-02-26 19:45 ` Richard Sandiford 2008-02-26 20:06 ` Jeff Law 0 siblings, 2 replies; 18+ messages in thread From: Peter Bergner @ 2008-02-26 19:04 UTC (permalink / raw) To: Richard Sandiford; +Cc: gcc-patches 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] PR35371 GCSE loses track of REG_POINTER attribute 2008-02-26 19:04 ` Peter Bergner @ 2008-02-26 19:45 ` Richard Sandiford 2008-02-26 20:06 ` Jeff Law 1 sibling, 0 replies; 18+ messages in thread From: Richard Sandiford @ 2008-02-26 19:45 UTC (permalink / raw) To: Peter Bergner; +Cc: gcc-patches 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] PR35371 GCSE loses track of REG_POINTER attribute 2008-02-26 19:04 ` Peter Bergner 2008-02-26 19:45 ` Richard Sandiford @ 2008-02-26 20:06 ` Jeff Law 2008-02-29 1:32 ` Peter Bergner 1 sibling, 1 reply; 18+ messages in thread From: Jeff Law @ 2008-02-26 20:06 UTC (permalink / raw) To: Peter Bergner; +Cc: Richard Sandiford, gcc-patches 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] PR35371 GCSE loses track of REG_POINTER attribute 2008-02-26 20:06 ` Jeff Law @ 2008-02-29 1:32 ` Peter Bergner 2008-03-03 19:17 ` Jeff Law 2008-03-10 15:32 ` [PING H.J. Lu] " Peter Bergner 0 siblings, 2 replies; 18+ messages in thread From: Peter Bergner @ 2008-02-29 1:32 UTC (permalink / raw) To: Jeff Law; +Cc: Richard Sandiford, gcc-patches, H.J. Lu [-- 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); ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] PR35371 GCSE loses track of REG_POINTER attribute 2008-02-29 1:32 ` Peter Bergner @ 2008-03-03 19:17 ` Jeff Law 2008-03-03 19:42 ` Peter Bergner 2008-03-10 15:32 ` [PING H.J. Lu] " Peter Bergner 1 sibling, 1 reply; 18+ messages in thread From: Jeff Law @ 2008-03-03 19:17 UTC (permalink / raw) To: Peter Bergner; +Cc: Richard Sandiford, gcc-patches, H.J. Lu 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] PR35371 GCSE loses track of REG_POINTER attribute 2008-03-03 19:17 ` Jeff Law @ 2008-03-03 19:42 ` Peter Bergner 2008-03-03 20:55 ` Jeff Law 0 siblings, 1 reply; 18+ messages in thread From: Peter Bergner @ 2008-03-03 19:42 UTC (permalink / raw) To: Jeff Law; +Cc: Richard Sandiford, gcc-patches, H.J. Lu 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] PR35371 GCSE loses track of REG_POINTER attribute 2008-03-03 19:42 ` Peter Bergner @ 2008-03-03 20:55 ` Jeff Law 0 siblings, 0 replies; 18+ messages in thread From: Jeff Law @ 2008-03-03 20:55 UTC (permalink / raw) To: Peter Bergner; +Cc: Richard Sandiford, gcc-patches, H.J. Lu 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PING H.J. Lu] Re: [PATCH] PR35371 GCSE loses track of REG_POINTER attribute 2008-02-29 1:32 ` Peter Bergner 2008-03-03 19:17 ` Jeff Law @ 2008-03-10 15:32 ` Peter Bergner 2008-03-10 16:22 ` H.J. Lu 2008-03-12 14:48 ` H.J. Lu 1 sibling, 2 replies; 18+ messages in thread From: Peter Bergner @ 2008-03-10 15:32 UTC (permalink / raw) To: H.J. Lu; +Cc: Richard Sandiford, gcc-patches, Jeff Law 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PING H.J. Lu] Re: [PATCH] PR35371 GCSE loses track of REG_POINTER attribute 2008-03-10 15:32 ` [PING H.J. Lu] " Peter Bergner @ 2008-03-10 16:22 ` H.J. Lu 2008-03-12 14:48 ` H.J. Lu 1 sibling, 0 replies; 18+ messages in thread From: H.J. Lu @ 2008-03-10 16:22 UTC (permalink / raw) To: Peter Bergner; +Cc: Richard Sandiford, gcc-patches, Jeff Law 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 > > > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PING H.J. Lu] Re: [PATCH] PR35371 GCSE loses track of REG_POINTER attribute 2008-03-10 15:32 ` [PING H.J. Lu] " Peter Bergner 2008-03-10 16:22 ` H.J. Lu @ 2008-03-12 14:48 ` H.J. Lu 2008-03-17 14:32 ` H.J. Lu 1 sibling, 1 reply; 18+ messages in thread From: H.J. Lu @ 2008-03-12 14:48 UTC (permalink / raw) To: Peter Bergner; +Cc: Richard Sandiford, gcc-patches, Jeff Law 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. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PING H.J. Lu] Re: [PATCH] PR35371 GCSE loses track of REG_POINTER attribute 2008-03-12 14:48 ` H.J. Lu @ 2008-03-17 14:32 ` H.J. Lu 0 siblings, 0 replies; 18+ messages in thread From: H.J. Lu @ 2008-03-17 14:32 UTC (permalink / raw) To: Peter Bergner; +Cc: Richard Sandiford, gcc-patches, Jeff Law [-- 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-- ^ permalink raw reply [flat|nested] 18+ messages in thread
[parent not found: <20080331225840.GA30393@vervain.rchland.ibm.com>]
* Re: [PATCH,updated] PR35371 GCSE loses track of REG_POINTER attribute [not found] <20080331225840.GA30393@vervain.rchland.ibm.com> @ 2008-04-05 7:49 ` Eric Botcazou 2008-04-07 18:08 ` Peter Bergner 0 siblings, 1 reply; 18+ messages in thread From: Eric Botcazou @ 2008-04-05 7:49 UTC (permalink / raw) To: Peter Bergner; +Cc: gcc-patches, Richard Sandiford, Jeff Law, H.J. Lu > This has bootstrapped and regtested with no errors on powerpc64-linux and > H.J. kindly SPEC tested this on x86_64 and saw no performance regressions. > Is this ok for mainline? It's OK if you find a better name for gen_reg_rtx_copy: the bare "copy" clearly suggests that it will copy the value of the source. Something like gen_reg_rtx_and_attrs for example but Richard may have a better idea. > Is it appropriate for gcc4.3? I'd say no: this is apparently not a regression and I think that touching commutative_operand_precedence should only be done with caution. > PR rtl-optimization/35371 > * rtlanal.c: Update copyright years. > (commutative_operand_precedence): Give SYMBOL_REF's the same precedence > as REG_POINTER and MEM_POINTER operands. > * emit-rtl.c (gen_reg_rtx_copy): New function. > (set_reg_attrs_from_value): Call mark_reg_pointer as appropriate. > * rtl.h (gen_reg_rtx_copy): Add prototype for new function. > * gcse.c: Update copyright years. > (pre_delete): Call gen_reg_rtx_copy. > (hoist_code): Likewise. > (build_store_vectors): Likewise. > (delete_store): Likewise. > * loop-invariant.c (move_invariant_reg): Likewise. > Update copyright years. -- Eric Botcazou ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH,updated] PR35371 GCSE loses track of REG_POINTER attribute 2008-04-05 7:49 ` [PATCH,updated] " Eric Botcazou @ 2008-04-07 18:08 ` Peter Bergner 0 siblings, 0 replies; 18+ messages in thread From: Peter Bergner @ 2008-04-07 18:08 UTC (permalink / raw) To: Eric Botcazou; +Cc: gcc-patches, Richard Sandiford, Jeff Law, H.J. Lu On Sat, 2008-04-05 at 09:36 +0200, Eric Botcazou wrote: > > This has bootstrapped and regtested with no errors on powerpc64-linux and > > H.J. kindly SPEC tested this on x86_64 and saw no performance regressions. > > Is this ok for mainline? > > It's OK if you find a better name for gen_reg_rtx_copy: the bare "copy" > clearly suggests that it will copy the value of the source. Something like > gen_reg_rtx_and_attrs for example but Richard may have a better idea. Actually, Richard and I both tried copping out which is how we ended up with the lame name I came up with. So your suggestion wins! :) I checked in the patch using your gcc_reg_rtx_and_attrs suggestion. If you change your mind, let me know and I'll change it. This is now on trunk as revision 133985. Thanks! Peter ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2008-04-07 17:42 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2008-02-25 22:30 [PATCH] PR35371 GCSE loses track of REG_POINTER attribute Peter Bergner 2008-02-25 23:13 ` Peter Bergner 2008-02-26 5:25 ` [PATCH,updated] " Peter Bergner 2008-02-26 5:49 ` [PATCH,withdrawn] " Peter Bergner 2008-02-26 18:00 ` [PATCH] " Richard Sandiford 2008-02-26 19:04 ` Peter Bergner 2008-02-26 19:45 ` Richard Sandiford 2008-02-26 20:06 ` Jeff Law 2008-02-29 1:32 ` Peter Bergner 2008-03-03 19:17 ` Jeff Law 2008-03-03 19:42 ` Peter Bergner 2008-03-03 20:55 ` Jeff Law 2008-03-10 15:32 ` [PING H.J. Lu] " Peter Bergner 2008-03-10 16:22 ` H.J. Lu 2008-03-12 14:48 ` H.J. Lu 2008-03-17 14:32 ` H.J. Lu [not found] <20080331225840.GA30393@vervain.rchland.ibm.com> 2008-04-05 7:49 ` [PATCH,updated] " Eric Botcazou 2008-04-07 18:08 ` Peter Bergner
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).