Re: IRA performance regressions on PPC
"H.J. Lu" <[EMAIL PROTECTED]> writes: > On Fri, Sep 5, 2008 at 10:24 AM, Vladimir Makarov <[EMAIL PROTECTED]> wrote: >> 1-3 days usually because gcc community and RA reviewers are very responsive. >> So I don't see a big difference in using ira-merge and trunk. I'd only >> recommend to apply patch >> >> http://gcc.gnu.org/ml/gcc-patches/2008-09/msg00427.html >> >> first because it is critical for performance but I don't know when it will >> be approved. >> > > I checked this patch into ira-merge branch. Hmm. Is that really a good idea? I think pretty much everyone is agreed that this patch shouldn't be applied to mainline in its current form (that is, without any insight into why the patch has such an effect on performance). And I'd got the impression the purpose of the branch was to hold patches that are fit for mainline. (Hey, has anyone ever had to argue so hard _against_ getting one of their patches applied? ;)) Richard
Re: PR37363: PR36090 and PR36182 all over again
Hans-Peter Nilsson wrote: >> Date: Fri, 5 Sep 2008 14:57:00 +0200 >> From: Hans-Peter Nilsson <[EMAIL PROTECTED]> > >> Maybe as part of a change from target macro to target hook, with >> LEGITIMATE_CONSTANT_P as a default would fit, even at this >> stage? > > Sorry, I mean CONSTANT_P, not LEGITIMATE_CONSTANT_P. Or maybe a > new macro or hook What about replacing the problematic uses of gen_rtx_CONST with "plus_constant (x, 0)"? plus_constant knows when to make a CONST rtx. There are just a handful of places where this would be needed: instead of the check after the wrong comment in cse.c, and everywhere gen_rtx_CONST is used in simplify-rtx.c. Paolo
Re: PR37363: PR36090 and PR36182 all over again
Paolo Bonzini wrote:
> Hans-Peter Nilsson wrote:
>>> Date: Fri, 5 Sep 2008 14:57:00 +0200
>>> From: Hans-Peter Nilsson <[EMAIL PROTECTED]>
>>> Maybe as part of a change from target macro to target hook, with
>>> LEGITIMATE_CONSTANT_P as a default would fit, even at this
>>> stage?
>> Sorry, I mean CONSTANT_P, not LEGITIMATE_CONSTANT_P. Or maybe a
>> new macro or hook
>
> What about replacing the problematic uses of gen_rtx_CONST with
> "plus_constant (x, 0)"? plus_constant knows when to make a CONST rtx.
>
> There are just a handful of places where this would be needed: instead
> of the check after the wrong comment in cse.c, and everywhere
> gen_rtx_CONST is used in simplify-rtx.c.
Here is a prototype patch, untested.
Paolo
2008-09-06 Paolo Bonzini <[EMAIL PROTECTED]>
* explow.c (plus_constant): Don't exit early if c == 0, to allow
canonicalizing CONSTs.
* cse.c (fold_rtx): Use plus_constant instead of wrapping with CONST.
* simplify-rtx.c (simplify_plus_minus): Likewise.
Index: cse.c
===
--- cse.c (revision 134435)
+++ cse.c (working copy)
@@ -3161,10 +3161,8 @@ fold_rtx (rtx x, rtx insn)
FIXME: those ports should be fixed. */
if (new != 0 && is_const
&& GET_CODE (new) == PLUS
- && (GET_CODE (XEXP (new, 0)) == SYMBOL_REF
- || GET_CODE (XEXP (new, 0)) == LABEL_REF)
&& GET_CODE (XEXP (new, 1)) == CONST_INT)
- new = gen_rtx_CONST (mode, new);
+ new = plus_constant (XEXP (new, 0), XEXP (new, 1));
+ else
+ new = plus_constant (new, 0);
}
break;
Index: simplify-rtx.c
===
--- simplify-rtx.c (revision 140055)
+++ simplify-rtx.c (working copy)
@@ -3625,7 +3625,7 @@ simplify_plus_minus (enum rtx_code code,
tem = simplify_binary_operation (ncode, mode, tem_lhs,
tem_rhs);
if (tem && !CONSTANT_P (tem))
- tem = gen_rtx_CONST (GET_MODE (tem), tem);
+ tem = plus_constant (tem, 0);
}
else
tem = simplify_binary_operation (ncode, mode, lhs, rhs);
@@ -3690,7 +3690,7 @@ simplify_plus_minus (enum rtx_code code,
&& GET_CODE (ops[i].op) == GET_CODE (ops[i - 1].op))
{
ops[i - 1].op = gen_rtx_MINUS (mode, ops[i - 1].op, ops[i].op);
- ops[i - 1].op = gen_rtx_CONST (mode, ops[i - 1].op);
+ ops[i - 1].op = plus_constant (ops[i - 1].op, 0);
if (i < n_ops - 1)
ops[i] = ops[i + 1];
n_ops--;
@@ -5247,7 +5247,7 @@ simplify_subreg (enum machine_mode outer
&& GET_MODE_BITSIZE (innermode) >= (2 * GET_MODE_BITSIZE (outermode))
&& GET_CODE (XEXP (op, 1)) == CONST_INT
&& (INTVAL (XEXP (op, 1)) & (GET_MODE_BITSIZE (outermode) - 1)) == 0
- && INTVAL (XEXP (op, 1)) < GET_MODE_BITSIZE (innermode)
+ && INTVAL (XEXP (op, 1)) < GET_MODE_BITSIZE (innermode)
&& byte == subreg_lowpart_offset (outermode, innermode))
{
int shifted_bytes = INTVAL (XEXP (op, 1)) / BITS_PER_UNIT;
Index: explow.c
===
--- explow.c(revision 134435)
+++ explow.c(working copy)
@@ -83,9 +83,6 @@ plus_constant (rtx x, HOST_WIDE_INT c)
rtx tem;
int all_constant = 0;
- if (c == 0)
-return x;
-
restart:
code = GET_CODE (x);
Re: PR37363: PR36090 and PR36182 all over again
Paolo Bonzini <[EMAIL PROTECTED]> writes:
> Index: cse.c
> ===
> --- cse.c (revision 134435)
> +++ cse.c (working copy)
> @@ -3161,10 +3161,8 @@ fold_rtx (rtx x, rtx insn)
> FIXME: those ports should be fixed. */
> if (new != 0 && is_const
> && GET_CODE (new) == PLUS
> - && (GET_CODE (XEXP (new, 0)) == SYMBOL_REF
> - || GET_CODE (XEXP (new, 0)) == LABEL_REF)
> && GET_CODE (XEXP (new, 1)) == CONST_INT)
> - new = gen_rtx_CONST (mode, new);
> + new = plus_constant (XEXP (new, 0), XEXP (new, 1));
> + else
> + new = plus_constant (new, 0);
>}
>break;
I'm not sure about this bit.
First of all, isn't the whole of the original "if" statement
redundant these days? "new" is the result of simplify_unary_operation,
and I'd have expected that function to return:
(const (plus [symbol_ref|label_ref] (const_int X)))
rather than:
(plus [symbol_ref|label_ref] (const_int X))
I think the same true of the revised "if" statement too: if plus_constant
could add a CONST, simplify_unary_operation should already have done that.
(The "else" bit isn't safe against "new" being NULL, btw.)
Couldn't:
{
int is_const = 0;
/* We can't simplify extension ops unless we know the
original mode. */
if ((code == ZERO_EXTEND || code == SIGN_EXTEND)
&& mode_arg0 == VOIDmode)
break;
/* If we had a CONST, strip it off and put it back later if we
fold. */
if (const_arg0 != 0 && GET_CODE (const_arg0) == CONST)
is_const = 1, const_arg0 = XEXP (const_arg0, 0);
new = simplify_unary_operation (code, mode,
const_arg0 ? const_arg0 : folded_arg0,
mode_arg0);
/* NEG of PLUS could be converted into MINUS, but that causes
expressions of the form
(CONST (MINUS (CONST_INT) (SYMBOL_REF)))
which many ports mistakenly treat as LEGITIMATE_CONSTANT_P.
FIXME: those ports should be fixed. */
if (new != 0 && is_const
&& GET_CODE (new) == PLUS
&& (GET_CODE (XEXP (new, 0)) == SYMBOL_REF
|| GET_CODE (XEXP (new, 0)) == LABEL_REF)
&& GET_CODE (XEXP (new, 1)) == CONST_INT)
new = gen_rtx_CONST (mode, new);
}
simply be replaced by:
/* We can't simplify extension ops unless we know the
original mode. */
if ((code == ZERO_EXTEND || code == SIGN_EXTEND)
&& mode_arg0 == VOIDmode)
break;
new = simplify_unary_operation (code, mode,
const_arg0 ? const_arg0 : folded_arg0,
mode_arg0);
?
(Sorry if I'm repeating earlier discussion here.)
Richard
Re: PR37363: PR36090 and PR36182 all over again
> I'm not sure about this bit. Couldn't [snip cse.c code]
> simply be replaced by:
>
> /* We can't simplify extension ops unless we know the
>original mode. */
> if ((code == ZERO_EXTEND || code == SIGN_EXTEND)
> && mode_arg0 == VOIDmode)
> break;
>
> new = simplify_unary_operation (code, mode,
> const_arg0 ? const_arg0 : folded_arg0,
> mode_arg0);
> ?
>
> (Sorry if I'm repeating earlier discussion here.)
I think so -- I was just trying to resemble the existing code as much as
possible (stage3), but it's probably better to clean up instead. What
do you thing about the simplify-rtx.c part instead?
Paolo
2008-09-06 Paolo Bonzini <[EMAIL PROTECTED]>
* cse.c (fold_rtx): Let simplify_unary_operation handle CONSTs.
* explow.c (plus_constant): Don't exit early if c == 0, to allow
canonicalizing CONSTs.
* simplify-rtx.c (simplify_plus_minus): Likewise.
Index: cse.c
===
--- cse.c (revision 134435)
+++ cse.c (working copy)
@@ -3138,33 +3138,20 @@ fold_rtx (rtx x, rtx insn)
{
case RTX_UNARY:
{
- int is_const = 0;
-
/* We can't simplify extension ops unless we know the
original mode. */
if ((code == ZERO_EXTEND || code == SIGN_EXTEND)
&& mode_arg0 == VOIDmode)
break;
- /* If we had a CONST, strip it off and put it back later if we
- fold. */
+ /* If we had a CONST, strip it off and let simplify_unary_operation
+ put it back if it can simplify something. */
if (const_arg0 != 0 && GET_CODE (const_arg0) == CONST)
- is_const = 1, const_arg0 = XEXP (const_arg0, 0);
+ const_arg0 = XEXP (const_arg0, 0);
new = simplify_unary_operation (code, mode,
const_arg0 ? const_arg0 : folded_arg0,
mode_arg0);
- /* NEG of PLUS could be converted into MINUS, but that causes
- expressions of the form
- (CONST (MINUS (CONST_INT) (SYMBOL_REF)))
- which many ports mistakenly treat as LEGITIMATE_CONSTANT_P.
- FIXME: those ports should be fixed. */
- if (new != 0 && is_const
- && GET_CODE (new) == PLUS
- && (GET_CODE (XEXP (new, 0)) == SYMBOL_REF
- || GET_CODE (XEXP (new, 0)) == LABEL_REF)
- && GET_CODE (XEXP (new, 1)) == CONST_INT)
- new = gen_rtx_CONST (mode, new);
}
break;
Index: simplify-rtx.c
===
--- simplify-rtx.c (revision 140055)
+++ simplify-rtx.c (working copy)
@@ -3625,7 +3625,7 @@ simplify_plus_minus (enum rtx_code code,
tem = simplify_binary_operation (ncode, mode, tem_lhs,
tem_rhs);
if (tem && !CONSTANT_P (tem))
- tem = gen_rtx_CONST (GET_MODE (tem), tem);
+ tem = plus_constant (tem, 0);
}
else
tem = simplify_binary_operation (ncode, mode, lhs, rhs);
@@ -3690,7 +3690,7 @@ simplify_plus_minus (enum rtx_code code,
&& GET_CODE (ops[i].op) == GET_CODE (ops[i - 1].op))
{
ops[i - 1].op = gen_rtx_MINUS (mode, ops[i - 1].op, ops[i].op);
- ops[i - 1].op = gen_rtx_CONST (mode, ops[i - 1].op);
+ ops[i - 1].op = plus_constant (ops[i - 1].op, 0);
if (i < n_ops - 1)
ops[i] = ops[i + 1];
n_ops--;
@@ -5247,7 +5247,7 @@ simplify_subreg (enum machine_mode outer
&& GET_MODE_BITSIZE (innermode) >= (2 * GET_MODE_BITSIZE (outermode))
&& GET_CODE (XEXP (op, 1)) == CONST_INT
&& (INTVAL (XEXP (op, 1)) & (GET_MODE_BITSIZE (outermode) - 1)) == 0
- && INTVAL (XEXP (op, 1)) < GET_MODE_BITSIZE (innermode)
+ && INTVAL (XEXP (op, 1)) < GET_MODE_BITSIZE (innermode)
&& byte == subreg_lowpart_offset (outermode, innermode))
{
int shifted_bytes = INTVAL (XEXP (op, 1)) / BITS_PER_UNIT;
Index: explow.c
===
--- explow.c(revision 134435)
+++ explow.c(working copy)
@@ -83,9 +83,6 @@ plus_constant (rtx x, HOST_WIDE_INT c)
rtx tem;
int all_constant = 0;
- if (c == 0)
-return x;
-
restart:
code = GET_CODE (x);
Re: PR37363: PR36090 and PR36182 all over again
Paolo Bonzini <[EMAIL PROTECTED]> writes: >> I'm not sure about this bit. Couldn't [snip cse.c code] >> simply be replaced by: >> >> /* We can't simplify extension ops unless we know the >> original mode. */ >> if ((code == ZERO_EXTEND || code == SIGN_EXTEND) >>&& mode_arg0 == VOIDmode) >> break; >> >> new = simplify_unary_operation (code, mode, >>const_arg0 ? const_arg0 : folded_arg0, >>mode_arg0); >> ? >> >> (Sorry if I'm repeating earlier discussion here.) > > I think so -- I was just trying to resemble the existing code as much as > possible (stage3), Understood. FWIW, if we have to apply this to a branch at any point, I'd be tempted to leave cse.c unchanged. > What do you thing about the simplify-rtx.c part instead? Hmm. Well... > Index: simplify-rtx.c > === > --- simplify-rtx.c(revision 140055) > +++ simplify-rtx.c(working copy) > @@ -3625,7 +3625,7 @@ simplify_plus_minus (enum rtx_code code, > tem = simplify_binary_operation (ncode, mode, tem_lhs, > tem_rhs); > > if (tem && !CONSTANT_P (tem)) > - tem = gen_rtx_CONST (GET_MODE (tem), tem); > + tem = plus_constant (tem, 0); > } > else > tem = simplify_binary_operation (ncode, mode, lhs, rhs); ...I'm not sure about this one. I think the same principle applies: if plus_constant _knows_ that something can be wrapped in a CONST, simplify_binary_operation should have given us the CONST to begin with. Also, the only cases that plus_constant can handle are CONST, SYMBOL_REF and LABEL_REF, all of which satisfy CONSTANT_P. So the new form ought to be dead on two counts. This code needs to handle cases that plus_constant doesn't know about. E.g. some UNSPECs are inherently constants, and the target can wrap those UNSPECs in a CONST. If we're given: (const (plus (unspec [...]) (const_int FOO))) and we strip the CONST (as we do here), we must be sure to add it back after meddling with the innards. It isn't immediately obvious to me why we need to strip CONSTs in the recursive call. I suppose it must have something to do with infinite recursion, but really, I think we should try to pass "canonical" rtl to simplify_binary_operation if possible. It feels like we might be breaking the infinite recursion in a less-than-ideal part of the code. Sorry for not offering a constructive alternative. I'd need to do some archaeology first. Richard
Re: Please, do not use the merged revisions log as the commit message when merging
2008/9/6 Christopher Faylor <[EMAIL PROTECTED]>: > >--- svn:log (original) >+++ svn:log Fri Sep 5 14:39:56 2008 >@@ -1,19740 +1,1 @@ >-Merged revisions > 136194,136196,136200,136209,136215-136217,136221-136222,1= > > 36229,136235-136239,136242,136247,136249,136251-136254,136266,136273,136276= > > -136277,136280,136282-136283,136291-136297,136301-136304,136308,136311,1363= > > 14-136315,136319-136323,136329,136331,136344,136348,136351,136355,136359-13= > > 6360,136362,136372,136374,136376-136378,136383,136386,136389,136395,136406,= > > 136416,136421,136423,136425,136427-136428,136431,136433-136435,136437-13643= > > 8,136485-136486,136491,136494,136497-136498,136503,136506,136513,136517-136= > > 518,136532,136534,136536-136537,136539-136542,136544-136547,136551,136554-1= > > 36555,136561-136563,136566-136569,136574,136576,136578,136580,136583,136585= > > ,136590,136596,136600-136602,136604-136605,136609,136615,136617,136619,1366= > > 32,136636,136639,136641,136645,136647-136651,136653-136655,136657-136658,13= >etc. > > This exhibited the same bounce behavior that we saw last time. > Well, that is a property change and it is surprising that the log shows the diff of the change. Normally logs only show what has been changed but not the diff. Neither John, nor I expected this behaviour. And normally the svn:log changes I suggested won't trigger this. This was a kind of special case. Perhaps bugzilla shouldn't track svn:log changes (specially since it cannot distinguish what is deleted and what is added). Cheers, Manuel.
Re: PR37363: PR36090 and PR36182 all over again
> if plus_constant _knows_ that something can be wrapped in a CONST,
> simplify_binary_operation should have given us the CONST to begin with.
> Also, the only cases that plus_constant can handle are CONST,
> SYMBOL_REF and LABEL_REF, all of which satisfy CONSTANT_P.
> So the new form ought to be dead on two counts.
Yes, and in the other case too:
ops[i - 1].op = gen_rtx_MINUS (mode, ops[i - 1].op, ops[i].op);
ops[i - 1].op = plus_constant (ops[i - 1].op, 0);
plus_constant won't understand the MINUS, and won't generate a CONST.
Still, having a new target hook for this seems overkill. For example,
since ports do have to deal with complicated constants when they expand
moves, and since some of them already look inside CONSTs in their
LEGITIMATE_CONSTANT_P, another possibility to throw in the air is
something like (better names welcome...)
rtx
avoid_terrible_constants (rtx x)
{
if (!CONSTANT_P (x))
x = gen_rtx_CONST (x);
/* If the target's move expanders will take care of it,
it must not be that bad. */
icode = optab_handler (mov_optab, GET_MODE (x))->insn_code;
if (*insn_data[icode].operand[1].predicate (x, GET_MODE (x)))
return x;
return NULL;
}
In case of cris, the predicate goes into general_operand, which does
if (CONSTANT_P (op))
return ((GET_MODE (op) == VOIDmode || GET_MODE (op) == mode
|| mode == VOIDmode)
&& (! flag_pic || LEGITIMATE_PIC_OPERAND_P (op))
&& LEGITIMATE_CONSTANT_P (op));
H-P can check for the problematic case inside his LEGITIMATE_CONSTANT_P
(*), or add a move expander for it.
(*) but then does this mean the documentation for L_C_P is obsolete,
and returning 1 is not necessarily a good thing to do for targets with
sections? Maybe there is a better definition that can be the default?
Anyway, at least how to use this function is pretty obvious:
tem_rhs = GET_CODE (rhs) == CONST ? XEXP (rhs, 0) : rhs;
tem = simplify_binary_operation (ncode, mode,
tem_lhs, tem_rhs);
- if (tem && !CONSTANT_P (tem))
- tem = gen_rtx_CONST (GET_MODE (tem), tem);
+ if (tem)
+ tem = avoid_terrible_constants (tem);
}
else
tem = simplify_binary_operation (ncode, mode, lhs, rhs);
...
&& CONSTANT_P (ops[i].op)
&& GET_CODE (ops[i].op) == GET_CODE (ops[i - 1].op))
{
- ops[i - 1].op = gen_rtx_MINUS (mode, ops[i - 1].op, ops[i].op);
- ops[i - 1].op = gen_rtx_CONST (mode, ops[i - 1].op);
- if (i < n_ops - 1)
- ops[i] = ops[i + 1];
- n_ops--;
+ rtx x;
+ x = gen_rtx_MINUS (mode, ops[i - 1].op, ops[i].op);
+ x = avoid_terrible_constants (x);
+ if (x)
+ {
+ ops[i - 1].op = x;
+ if (i < n_ops - 1)
+ ops[i] = ops[i + 1];
+ n_ops--;
+}
}
if (n_ops > 1
I'm absolutely unsure that this is the way to go; but it has two
advantages: 1) not leaking really bad constants outside simplify-rtx.c;
2) it makes clear how to fix bugs -- you restrict
LEGITIMATE_CONSTANT_P/LEGITIMATE_PIC_OPERAND_P or add a move expander.
Paolo
Re: PR37363: PR36090 and PR36182 all over again
> Date: Sat, 06 Sep 2008 12:50:18 +0200 > From: Paolo Bonzini <[EMAIL PROTECTED]> > Still, having a new target hook for this seems overkill. But it's better than abusing an old macro with a slightly different use. > For example, > since ports do have to deal with complicated constants when they expand > moves, and since some of them already look inside CONSTs in their > LEGITIMATE_CONSTANT_P, another possibility to throw in the air is > something like (better names welcome...) > > rtx > avoid_terrible_constants (rtx x) valid_const_contents (name adjusted for use as per below). > In case of cris, the predicate goes into general_operand, which does > > if (CONSTANT_P (op)) > return ((GET_MODE (op) == VOIDmode || GET_MODE (op) == mode > || mode == VOIDmode) > && (! flag_pic || LEGITIMATE_PIC_OPERAND_P (op)) > && LEGITIMATE_CONSTANT_P (op)); > > H-P can check for the problematic case inside his LEGITIMATE_CONSTANT_P > (*), or add a move expander for it. I think you're mixing up CRIS and rs6000, the latter which generated something it had to handle but which was munged, PR36090. CRIS is mainstream in that sense. (You'd have to get buy-in from David Edelsohn on a LEGITIMATE_CONSTANT_P definition in rs6000 if PR36090 resurfaces.) Did you mean this as a short-term or long-term solution? (Mind, we already have a proposed short-term solution.) > (*) but then does this mean the documentation for L_C_P is obsolete, > and returning 1 is not necessarily a good thing to do for targets with > sections? Maybe there is a better definition that can be the default? Again, LEGITIMATE_CONSTANT_P is the wrong macro, it's for checking constants which are appropriate as immediate operands (to non-move insns), not for being at-all-legitimate. Signalling that they are not legitimate means they can still be handled by a move. Not only the documentation, but, again, you'd have to replace all its uses, because current users just call force_reg or force_const_mem for a negative! And then perhaps you'd have to invent a new macro for the old documented behavior (...which might be just as good, considering the confusing name). > I'm absolutely unsure that this is the way to go; but it has two > advantages: 1) not leaking really bad constants outside simplify-rtx.c; We can do that. > 2) it makes clear how to fix bugs -- you restrict > LEGITIMATE_CONSTANT_P/LEGITIMATE_PIC_OPERAND_P or add a move expander. Contradicting current use, where anything that's found in a non-LEGITIMATE_CONSTANT_P/LEGITIMATE_PIC_OPERAND_P must be handled by a move expander! To wit: a new bug would surface: you could here form something that wasn't LEGITIMATE_CONSTANT_P but which was handled by a move expander, and you'd force this into an insn which *isn't* a move. N.B. the insn in PR36182 wasn't a move. (FWIW, I'll add a LEGITIMATE_CONSTANT_P to CRIS just to cover my bases. It won't solve the basic problem, because that could just cause that invalid CONST contents in PR37363 and PR36182 to end up in a move insn instead.) Let's just have the new target hook and use it in *this* place (valid_const_contents) for a starter. For now, the default always returning "yup, that CONST-candidate you're trying to form is valid". I can write a candidate to use with CRIS but usable for mainstream ELF targets to use (though it'd have to refuse sym2-sym1 expressions where sym1 is in the current section). brgds, H-P
Re: PR37363: PR36090 and PR36182 all over again
>> In case of cris, the predicate goes into general_operand, which does >> >> if (CONSTANT_P (op)) >> return ((GET_MODE (op) == VOIDmode || GET_MODE (op) == mode >> || mode == VOIDmode) >> && (! flag_pic || LEGITIMATE_PIC_OPERAND_P (op)) >> && LEGITIMATE_CONSTANT_P (op)); >> >> H-P can check for the problematic case inside his LEGITIMATE_CONSTANT_P >> (*), or add a move expander for it. > > I think you're mixing up CRIS and rs6000, the latter which > generated something it had to handle but which was munged, > PR36090. CRIS is mainstream in that sense. (You'd have to get > buy-in from David Edelsohn on a LEGITIMATE_CONSTANT_P definition > in rs6000 if PR36090 resurfaces.) This is from CRIS: (define_expand "movsi" [(set (match_operand:SI 0 "nonimmediate_operand" "") (match_operand:SI 1 "cris_general_operand_or_symbol" ""))] ... (define_special_predicate "cris_general_operand_or_symbol" (ior (match_operand 0 "general_operand") (and (match_code "const, symbol_ref, label_ref") ... > Did you mean this as a short-term or long-term solution? > (Mind, we already have a proposed short-term solution.) As a long term solution. Though not in that exact shape -- I wanted to have discussion on it and converge together to a real solution. >> (*) but then does this mean the documentation for L_C_P is obsolete, >> and returning 1 is not necessarily a good thing to do for targets with >> sections? Maybe there is a better definition that can be the default? > > Again, LEGITIMATE_CONSTANT_P is the wrong macro, it's for > checking constants which are appropriate as immediate operands > (to non-move insns), not for being at-all-legitimate. LEGITIMATE_CONSTANT_P is just what is used by general_operand. I'm proposing another use of *the predicate for mov's operand 1*, not of LEGITIMATE_CONSTANT_P. With the above questions, I was expressing my doubts on the doc for LEGITIMATE_CONSTANT_P in general. > Signalling that they are not legitimate means they can still be > handled by a move. That's why I used the predicate. >> 2) it makes clear how to fix bugs -- you restrict >> LEGITIMATE_CONSTANT_P/LEGITIMATE_PIC_OPERAND_P or add a move expander. > > Contradicting current use, where anything that's found in a > non-LEGITIMATE_CONSTANT_P/LEGITIMATE_PIC_OPERAND_P must be > handled by a move expander! Not necessarily; anything that's found in a non-legitimate constant must be handled by force_reg, and force_reg also tries using force_operand if what it gets is not a general_operand. But maybe it's necessary to add a if (GET_CODE (value) == CONST) value = XEXP (value, 0); in force_operand. > To wit: a new bug would surface: you could here form something > that wasn't LEGITIMATE_CONSTANT_P but which was handled by a > move expander, and you'd force this into an insn which *isn't* a > move. N.B. the insn in PR36182 wasn't a move. Shouldn't the insn fail recognization, then? > (FWIW, I'll add a LEGITIMATE_CONSTANT_P to CRIS just to cover my > bases. It won't solve the basic problem, because that could > just cause that invalid CONST contents in PR37363 and PR36182 to > end up in a move insn instead.) I don't think so, because general_operand would pass the CONST to your LEGITIMATE_CONSTANT_P, and hence cause it to be rejected. Paolo
Re: IRA performance regressions on PPC
On Fri, 2008-09-05 at 10:34 -0700, H.J. Lu wrote: > On Fri, Sep 5, 2008 at 10:24 AM, Vladimir Makarov <[EMAIL PROTECTED]> wrote: > > > > H.J. Lu keeps ira-branch merge more fresh than trunk. But the lag is only > > I won't apply any non-IRA related patches to ira-merge branch so > that you can get a fair comparison for IRA without regressions > introduced by other changes. > > > 1-3 days usually because gcc community and RA reviewers are very responsive. > > So I don't see a big difference in using ira-merge and trunk. I'd only > > recommend to apply patch > > > > http://gcc.gnu.org/ml/gcc-patches/2008-09/msg00427.html > > > > first because it is critical for performance but I don't know when it will > > be approved. > > > > I checked this patch into ira-merge branch. I've verifired applu and facerec against the ira-merge branch. The numbers are just as bad as trunk. So, apparently, the last patch didn't improve performance on those benchmarks. Any other ideas you'd like me to try on IRA? Thanks, Luis
Re: IRA performance regressions on PPC
On Sat, Sep 6, 2008 at 12:47 AM, Richard Sandiford <[EMAIL PROTECTED]> wrote: > "H.J. Lu" <[EMAIL PROTECTED]> writes: >> On Fri, Sep 5, 2008 at 10:24 AM, Vladimir Makarov <[EMAIL PROTECTED]> wrote: >>> 1-3 days usually because gcc community and RA reviewers are very responsive. >>> So I don't see a big difference in using ira-merge and trunk. I'd only >>> recommend to apply patch >>> >>> http://gcc.gnu.org/ml/gcc-patches/2008-09/msg00427.html >>> >>> first because it is critical for performance but I don't know when it will >>> be approved. >>> >> >> I checked this patch into ira-merge branch. > > Hmm. Is that really a good idea? I think pretty much everyone > is agreed that this patch shouldn't be applied to mainline in > its current form (that is, without any insight into why the patch > has such an effect on performance). And I'd got the impression the > purpose of the branch was to hold patches that are fit for mainline. > > (Hey, has anyone ever had to argue so hard _against_ getting > one of their patches applied? ;)) > The purpose of ira-merge branch is to make sure that IRA is really OK. I applied your patch so that people can verify if it makes any difference at all. -- H.J.
Re: IRA performance regressions on PPC
On Sat, Sep 6, 2008 at 7:05 AM, Luis Machado <[EMAIL PROTECTED]> wrote: > > On Fri, 2008-09-05 at 10:34 -0700, H.J. Lu wrote: >> On Fri, Sep 5, 2008 at 10:24 AM, Vladimir Makarov <[EMAIL PROTECTED]> wrote: >> > >> > H.J. Lu keeps ira-branch merge more fresh than trunk. But the lag is only >> >> I won't apply any non-IRA related patches to ira-merge branch so >> that you can get a fair comparison for IRA without regressions >> introduced by other changes. >> >> > 1-3 days usually because gcc community and RA reviewers are very >> > responsive. >> > So I don't see a big difference in using ira-merge and trunk. I'd only >> > recommend to apply patch >> > >> > http://gcc.gnu.org/ml/gcc-patches/2008-09/msg00427.html >> > >> > first because it is critical for performance but I don't know when it will >> > be approved. >> > >> >> I checked this patch into ira-merge branch. > > I've verifired applu and facerec against the ira-merge branch. The > numbers are just as bad as trunk. So, apparently, the last patch didn't > improve performance on those benchmarks. > > Any other ideas you'd like me to try on IRA? > > Can you verify if your problem is related to http://gcc.gnu.org/bugzilla/show_bug.cgi?id=28690 -- H.J.
Re: Bootstrap failure on sparc-sun-solaris2.10
H.J. Lu wrote: On Fri, Sep 5, 2008 at 2:57 PM, Andreas Tobler <[EMAIL PROTECTED]> wrote: H.J. Lu wrote: It is a temporary branch, branches/ira-merge, to track IRA related problems. Same issue. Does trunk bootstrap with revision 139589? No, gcc version 4.4.0 20080905 (experimental) [trunk revision 140042] (GCC) If trunk was broken at revision 139589, your problem isn't related to IRA since IRA was merged at revision 139590. Sorry, misread the above. - 139589 bootstrap ok and tested - 139590 bootstrap nok. Andreas
Re: Bootstrap failure on sparc-sun-solaris2.10
On Sat, Sep 6, 2008 at 8:30 AM, Andreas Tobler <[EMAIL PROTECTED]> wrote: > H.J. Lu wrote: >> >> On Fri, Sep 5, 2008 at 2:57 PM, Andreas Tobler <[EMAIL PROTECTED]> >> wrote: >>> >>> H.J. Lu wrote: >>> >> It is a temporary branch, branches/ira-merge, to track >> IRA related problems. >> > Same issue. Does trunk bootstrap with revision 139589? >>> No, gcc version 4.4.0 20080905 (experimental) [trunk revision 140042] >>> (GCC) >>> >> >> If trunk was broken at revision 139589, your problem isn't >> related to IRA since IRA was merged at revision 139590. > > Sorry, misread the above. > > - 139589 bootstrap ok and tested > - 139590 bootstrap nok. > I suggest you open a bug report saying that IRA breaks Sparc bootstrap. -- H.J.
Re: Please, do not use the merged revisions log as the commit message when merging
On Sat, 6 Sep 2008, Manuel López-Ibáñez wrote: > Well, that is a property change and it is surprising that the log > shows the diff of the change. Normally logs only show what has been > changed but not the diff. Neither John, nor I expected this behaviour. Changes to *revision* properties deliberately show the diff because revision properties are not versioned so sending the diff to gcc-cvs provides the only audit trail for such changes in case mistakes are made. However, this case illustrates that such diffs should only go to gcc-cvs and not be used to extract PR numbers for gcc-bugzilla. -- Joseph S. Myers [EMAIL PROTECTED]
Re: IRA performance regressions on PPC
On Sat, 2008-09-06 at 07:49 -0700, H.J. Lu wrote: > On Sat, Sep 6, 2008 at 7:05 AM, Luis Machado <[EMAIL PROTECTED]> wrote: > > > > On Fri, 2008-09-05 at 10:34 -0700, H.J. Lu wrote: > >> On Fri, Sep 5, 2008 at 10:24 AM, Vladimir Makarov <[EMAIL PROTECTED]> > >> wrote: > >> > > >> > H.J. Lu keeps ira-branch merge more fresh than trunk. But the lag is > >> > only > >> > >> I won't apply any non-IRA related patches to ira-merge branch so > >> that you can get a fair comparison for IRA without regressions > >> introduced by other changes. > >> > >> > 1-3 days usually because gcc community and RA reviewers are very > >> > responsive. > >> > So I don't see a big difference in using ira-merge and trunk. I'd only > >> > recommend to apply patch > >> > > >> > http://gcc.gnu.org/ml/gcc-patches/2008-09/msg00427.html > >> > > >> > first because it is critical for performance but I don't know when it > >> > will > >> > be approved. > >> > > >> > >> I checked this patch into ira-merge branch. > > > > I've verifired applu and facerec against the ira-merge branch. The > > numbers are just as bad as trunk. So, apparently, the last patch didn't > > improve performance on those benchmarks. > > > > Any other ideas you'd like me to try on IRA? > > > > > > Can you verify if your problem is related to > > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=28690 Even with data that shows us that the degradation occured between revisions 139589 and 139590 (139589 showing good numbers and 139590 showing bad numbers), would it still make sense to consider ticket 28690 as the possible reason for this degradation? Thanks, Luis
Re: IRA performance regressions on PPC
On Sat, Sep 6, 2008 at 11:24 AM, Luis Machado
<[EMAIL PROTECTED]> wrote:
>
> On Sat, 2008-09-06 at 07:49 -0700, H.J. Lu wrote:
>> On Sat, Sep 6, 2008 at 7:05 AM, Luis Machado <[EMAIL PROTECTED]> wrote:
>> >
>> > On Fri, 2008-09-05 at 10:34 -0700, H.J. Lu wrote:
>> >> On Fri, Sep 5, 2008 at 10:24 AM, Vladimir Makarov <[EMAIL PROTECTED]>
>> >> wrote:
>> >> >
>> >> > H.J. Lu keeps ira-branch merge more fresh than trunk. But the lag is
>> >> > only
>> >>
>> >> I won't apply any non-IRA related patches to ira-merge branch so
>> >> that you can get a fair comparison for IRA without regressions
>> >> introduced by other changes.
>> >>
>> >> > 1-3 days usually because gcc community and RA reviewers are very
>> >> > responsive.
>> >> > So I don't see a big difference in using ira-merge and trunk. I'd only
>> >> > recommend to apply patch
>> >> >
>> >> > http://gcc.gnu.org/ml/gcc-patches/2008-09/msg00427.html
>> >> >
>> >> > first because it is critical for performance but I don't know when it
>> >> > will
>> >> > be approved.
>> >> >
>> >>
>> >> I checked this patch into ira-merge branch.
>> >
>> > I've verifired applu and facerec against the ira-merge branch. The
>> > numbers are just as bad as trunk. So, apparently, the last patch didn't
>> > improve performance on those benchmarks.
>> >
>> > Any other ideas you'd like me to try on IRA?
>> >
>> >
>>
>> Can you verify if your problem is related to
>>
>> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=28690
>
> Even with data that shows us that the degradation occured between
> revisions 139589 and 139590 (139589 showing good numbers and 139590
> showing bad numbers), would it still make sense to consider ticket 28690
> as the possible reason for this degradation?
>
>
My understanding is PowerPC is quite sensitive to choice of register
as shown in PR 28690. IRA merge may make fixes for PR 28690
ineffective. There are a few small testcases in PR 28690. You can
check if those problems in PR 28690 come back due to IRA merge.
Also, IRA disables regmove:
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=37364
I don't know its impact on PowerPC. Can you try
--- ./regmove.c.regmove 2008-09-06 10:09:43.0 -0700
+++ ./regmove.c 2008-09-06 11:34:24.0 -0700
@@ -1117,8 +1117,7 @@ regmove_optimize (rtx f, int nregs)
for (pass = 0; pass <= 2; pass++)
{
- /* We need fewer optimizations for IRA. */
- if ((! flag_regmove || flag_ira) && pass >= flag_expensive_optimizations)
+ if ((! flag_regmove) && pass >= flag_expensive_optimizations)
goto done;
if (dump_file)
@@ -1167,8 +1166,7 @@ regmove_optimize (rtx f, int nregs)
}
}
- /* All optimizations important for IRA have been done. */
- if (! flag_regmove || flag_ira)
+ if (! flag_regmove)
continue;
if (! find_matches (insn, &match))
on the current ira-merge branch.
--
H.J.
Re: IRA performance regressions on PPC
H.J. Lu wrote:
On Sat, Sep 6, 2008 at 11:24 AM, Luis Machado
<[EMAIL PROTECTED]> wrote:
On Sat, 2008-09-06 at 07:49 -0700, H.J. Lu wrote:
On Sat, Sep 6, 2008 at 7:05 AM, Luis Machado <[EMAIL PROTECTED]> wrote:
On Fri, 2008-09-05 at 10:34 -0700, H.J. Lu wrote:
On Fri, Sep 5, 2008 at 10:24 AM, Vladimir Makarov <[EMAIL PROTECTED]> wrote:
H.J. Lu keeps ira-branch merge more fresh than trunk. But the lag is only
I won't apply any non-IRA related patches to ira-merge branch so
that you can get a fair comparison for IRA without regressions
introduced by other changes.
1-3 days usually because gcc community and RA reviewers are very responsive.
So I don't see a big difference in using ira-merge and trunk. I'd only
recommend to apply patch
http://gcc.gnu.org/ml/gcc-patches/2008-09/msg00427.html
first because it is critical for performance but I don't know when it will
be approved.
I checked this patch into ira-merge branch.
I've verifired applu and facerec against the ira-merge branch. The
numbers are just as bad as trunk. So, apparently, the last patch didn't
improve performance on those benchmarks.
Any other ideas you'd like me to try on IRA?
Can you verify if your problem is related to
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=28690
Even with data that shows us that the degradation occured between
revisions 139589 and 139590 (139589 showing good numbers and 139590
showing bad numbers), would it still make sense to consider ticket 28690
as the possible reason for this degradation?
My understanding is PowerPC is quite sensitive to choice of register
as shown in PR 28690. IRA merge may make fixes for PR 28690
ineffective. There are a few small testcases in PR 28690. You can
check if those problems in PR 28690 come back due to IRA merge.
Also, IRA disables regmove:
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=37364
I don't know its impact on PowerPC. Can you try
--- ./regmove.c.regmove 2008-09-06 10:09:43.0 -0700
+++ ./regmove.c 2008-09-06 11:34:24.0 -0700
@@ -1117,8 +1117,7 @@ regmove_optimize (rtx f, int nregs)
for (pass = 0; pass <= 2; pass++)
{
- /* We need fewer optimizations for IRA. */
- if ((! flag_regmove || flag_ira) && pass >= flag_expensive_optimizations)
+ if ((! flag_regmove) && pass >= flag_expensive_optimizations)
goto done;
if (dump_file)
@@ -1167,8 +1166,7 @@ regmove_optimize (rtx f, int nregs)
}
}
- /* All optimizations important for IRA have been done. */
- if (! flag_regmove || flag_ira)
+ if (! flag_regmove)
continue;
if (! find_matches (insn, &match))
on the current ira-merge branch.
I can't express how badly I feel this is the wrong direction to be
taking.Remove needs to go away and we need to be looking at the root
failures rather than re-enabling this crap code in regmove.c
I've got a performance regression as well that ties into the disabling
of regmove, but doing a root cause analysis has made it plainly clear
that the problem is not regmove, nor IRA, nor the backend port in
question. For my specific problem the root cause is actually subreg
lowering.While I could fix my regression by twiddling regmove and/or
the port itself, neither change is actually solving the problem. I
would *STRONGLY* suggest you take the time to do a root cause analysis
or at least avoid installing this bandaid patch.
Jeff
Re: Please, do not use the merged revisions log as the commit message when merging
Feel free to edit the hook scripts to do this. On Sat, Sep 6, 2008 at 1:26 PM, Joseph S. Myers <[EMAIL PROTECTED]> wrote: > On Sat, 6 Sep 2008, Manuel López-Ibáñez wrote: > >> Well, that is a property change and it is surprising that the log >> shows the diff of the change. Normally logs only show what has been >> changed but not the diff. Neither John, nor I expected this behaviour. > > Changes to *revision* properties deliberately show the diff because > revision properties are not versioned so sending the diff to gcc-cvs > provides the only audit trail for such changes in case mistakes are made. > However, this case illustrates that such diffs should only go to gcc-cvs > and not be used to extract PR numbers for gcc-bugzilla. > > -- > Joseph S. Myers > [EMAIL PROTECTED]
