* 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; 3+ 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] 3+ messages in thread
* Re: [PATCH,updated] PR35371 GCSE loses track of REG_POINTER attribute
2008-04-05 7:49 ` [PATCH,updated] PR35371 GCSE loses track of REG_POINTER attribute Eric Botcazou
@ 2008-04-07 18:08 ` Peter Bergner
0 siblings, 0 replies; 3+ 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] 3+ messages in thread
* [PATCH] PR35371 GCSE loses track of REG_POINTER attribute
@ 2008-02-25 22:30 Peter Bergner
2008-02-26 5:25 ` [PATCH,updated] " Peter Bergner
0 siblings, 1 reply; 3+ 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] 3+ messages in thread
* Re: [PATCH,updated] PR35371 GCSE loses track of REG_POINTER attribute
2008-02-25 22:30 [PATCH] " Peter Bergner
@ 2008-02-26 5:25 ` Peter Bergner
0 siblings, 0 replies; 3+ 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] 3+ messages in thread
end of thread, other threads:[~2008-04-07 17:42 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20080331225840.GA30393@vervain.rchland.ibm.com>
2008-04-05 7:49 ` [PATCH,updated] PR35371 GCSE loses track of REG_POINTER attribute Eric Botcazou
2008-04-07 18:08 ` Peter Bergner
2008-02-25 22:30 [PATCH] " Peter Bergner
2008-02-26 5:25 ` [PATCH,updated] " 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).