| Message ID | 55A7CCDA.8050203@arm.com |
|---|---|
| State | New |
| Headers | show |
On Thu, Jul 16, 2015 at 04:25:14PM +0100, Kyrill Tkachov wrote: > Hi all, > > This is an attempt to solve the problem in the thread starting at > https://gcc.gnu.org/ml/gcc-patches/2015-07/msg01010.html > in a generic way after some pointers from Segher and Andrew. > > The problem I got was that combine_simplify_rtx was trying to > do some special handling of unary operations applied to if_then_else > but ended up exiting early due to: > > enum rtx_code cond_code = simplify_comparison (NE, &cond, &cop1); > > if (cond_code == NE && COMPARISON_P (cond)) > return x; > > I tried removing that bug that led to regressions in SPEC2006. > The solution that worked for me led to two patches. > > In this first patch we add a simplification step to the rtx before trying > any substitutions. > diff --git a/gcc/combine.c b/gcc/combine.c > index 574f874..40d2231 100644 > --- a/gcc/combine.c > +++ b/gcc/combine.c > @@ -5510,6 +5510,17 @@ combine_simplify_rtx (rtx x, machine_mode op0_mode, int in_dest, > { > rtx cond, true_rtx, false_rtx; > > + /* If some simplification is possible from the start, try it now. */ > + temp = simplify_rtx (x); > + > + if (temp) > + { > + x = temp; > + code = GET_CODE (x); > + mode = GET_MODE (x); > + op0_mode = VOIDmode; > + } > + > cond = if_then_else_cond (x, &true_rtx, &false_rtx); > if (cond != 0 > /* If everything is a comparison, what we have is highly unlikely If you always want to simplify first, does it work to move this whole big block behind the simplify just following it? Or do you want to simplify after the transform as well? Segher
On 16/07/15 19:13, Segher Boessenkool wrote: > On Thu, Jul 16, 2015 at 04:25:14PM +0100, Kyrill Tkachov wrote: >> Hi all, >> >> This is an attempt to solve the problem in the thread starting at >> https://gcc.gnu.org/ml/gcc-patches/2015-07/msg01010.html >> in a generic way after some pointers from Segher and Andrew. >> >> The problem I got was that combine_simplify_rtx was trying to >> do some special handling of unary operations applied to if_then_else >> but ended up exiting early due to: >> >> enum rtx_code cond_code = simplify_comparison (NE, &cond, &cop1); >> >> if (cond_code == NE && COMPARISON_P (cond)) >> return x; >> >> I tried removing that bug that led to regressions in SPEC2006. >> The solution that worked for me led to two patches. >> >> In this first patch we add a simplification step to the rtx before trying >> any substitutions. >> diff --git a/gcc/combine.c b/gcc/combine.c >> index 574f874..40d2231 100644 >> --- a/gcc/combine.c >> +++ b/gcc/combine.c >> @@ -5510,6 +5510,17 @@ combine_simplify_rtx (rtx x, machine_mode op0_mode, int in_dest, >> { >> rtx cond, true_rtx, false_rtx; >> >> + /* If some simplification is possible from the start, try it now. */ >> + temp = simplify_rtx (x); >> + >> + if (temp) >> + { >> + x = temp; >> + code = GET_CODE (x); >> + mode = GET_MODE (x); >> + op0_mode = VOIDmode; >> + } >> + >> cond = if_then_else_cond (x, &true_rtx, &false_rtx); >> if (cond != 0 >> /* If everything is a comparison, what we have is highly unlikely > If you always want to simplify first, does it work to move this whole big > block behind the simplify just following it? Or do you want to simplify > after the transform as well? You mean move this hunk outside the "if (BINARY_P (x)...)" block it's in? I think it would work, but I'm not sure if it would affect other cases. I was also conscious that simplify_rtx might not be a cheap function to call so frequently (or is it? I didn't profile it), so I tried to avoid calling it unless I need for the transformation in question here. Kyrill > > > Segher >
On Thu, Jul 16, 2015 at 07:17:54PM +0100, Kyrill Tkachov wrote: > >If you always want to simplify first, does it work to move this whole big > >block behind the simplify just following it? Or do you want to simplify > >after the transform as well? > > You mean move this hunk outside the "if (BINARY_P (x)...)" block it's in? > I think it would work, but I'm not sure if it would affect other cases. > I was also conscious that simplify_rtx might not be a cheap function to call > so frequently (or is it? I didn't profile it), so I tried to avoid calling > it unless I need for the transformation in question here. I mean move the whole "if (BINARY_P ..." block to after the existing simplify calls, to just before the "First see if we can apply" comment, and not do a new simplify_rtx call at all. Does that work? Which brings the question why it wasn't there in the first place, hrm. Segher
On 16/07/15 19:28, Segher Boessenkool wrote: > On Thu, Jul 16, 2015 at 07:17:54PM +0100, Kyrill Tkachov wrote: >>> If you always want to simplify first, does it work to move this whole big >>> block behind the simplify just following it? Or do you want to simplify >>> after the transform as well? >> You mean move this hunk outside the "if (BINARY_P (x)...)" block it's in? >> I think it would work, but I'm not sure if it would affect other cases. >> I was also conscious that simplify_rtx might not be a cheap function to call >> so frequently (or is it? I didn't profile it), so I tried to avoid calling >> it unless I need for the transformation in question here. > I mean move the whole "if (BINARY_P ..." block to after the existing > simplify calls, to just before the "First see if we can apply" comment, > and not do a new simplify_rtx call at all. Does that work? Yes, it does the transformation I want :) if it's combined (pardon the pun) with the simplify-rtx.c patch at https://gcc.gnu.org/ml/gcc-patches/2015-07/msg01433.html > > Which brings the question why it wasn't there in the first place, hrm. Dunno, I'll test this approach more thoroughly tomorrow, check the impact on some codebases and propose a patch if it all works out. Thanks for the help, Kyrill > > > Segher >
commit 685bc1a66a36292329f678bae555e9c43e434e5d
Author: Kyrylo Tkachov <kyrylo.tkachov@arm.com>
Date: Thu Jul 9 16:54:23 2015 +0100
[combine] Try to simplify before substituting
diff --git a/gcc/combine.c b/gcc/combine.c
index 574f874..40d2231 100644
--- a/gcc/combine.c
+++ b/gcc/combine.c
@@ -5510,6 +5510,17 @@ combine_simplify_rtx (rtx x, machine_mode op0_mode, int in_dest,
{
rtx cond, true_rtx, false_rtx;
+ /* If some simplification is possible from the start, try it now. */
+ temp = simplify_rtx (x);
+
+ if (temp)
+ {
+ x = temp;
+ code = GET_CODE (x);
+ mode = GET_MODE (x);
+ op0_mode = VOIDmode;
+ }
+
cond = if_then_else_cond (x, &true_rtx, &false_rtx);
if (cond != 0
/* If everything is a comparison, what we have is highly unlikely
Hi all, This is an attempt to solve the problem in the thread starting at https://gcc.gnu.org/ml/gcc-patches/2015-07/msg01010.html in a generic way after some pointers from Segher and Andrew. The problem I got was that combine_simplify_rtx was trying to do some special handling of unary operations applied to if_then_else but ended up exiting early due to: enum rtx_code cond_code = simplify_comparison (NE, &cond, &cop1); if (cond_code == NE && COMPARISON_P (cond)) return x; I tried removing that bug that led to regressions in SPEC2006. The solution that worked for me led to two patches. In this first patch we add a simplification step to the rtx before trying any substitutions. In the second patch I add the simplify-rtx.c simplification to transform - (y ? -x : x) into (!y ? -x : x) which fixes the testcase I mentioned in the first thread. This first patch by itself already showed to be an improvement for aarch64 with by managing to eliminate a large amount of redundant zero_extend operations in SPEC2006. Overall, I saw a 2.8% decrease in [su]xt[bhw] instructions generated for the whole of SPEC2006 and no regressions in code quality i.e. no instructions that were combined before but not combine with this patch. Bootstrapped and tested on arm, aarch64 and x86_64. Ok for trunk? Thanks, Kyrill 2015-07-16 Kyrylo Tkachov <kyrylo.tkachov@arm.com> * combine.c (combine_simplify_rtx): Try to simplify if_then_else rtxes before trying substitutions.