[PATCH] combine: Disregard clobbers in another test for two SETs (PR65693)

[PATCH] combine: Disregard clobbers in another test for two SETs (PR65693)

[Segher Boessenkool]

PR65693 exposes a case where combine does a worse job after my patches
to split parallels before combining.  We start with a parallel of an
udiv and an umod, and a clobber; the umod is dead.  The instruction
is combined with one setting the divisor pseudo to a power-of-two
constant, so we end up with a parallel of an lshiftrt, an and (dead),
and a clobber.  This is not a recognised instruction.

Before my patches this was a 2->1 combination, and the combiner throws
away the dead set and everyone is happy.  After the patches, this now
is a 3->1 combination, the combiner does not throw away the dead set
but tries to split the parallel into two, which does not work because
one of the resulting insns has to end up as i2, which is earlier than
the original sets.  The combiner gives up.

There already is code to throw away dead sets in the 3->1 case, but
it only works for a parallel for two sets without any clobbers.  This
patch fixes it.

Tested on powerpc64-linux (-m32,-m32/-mpowerpc64,-m64,-m64/-mlra);
no regressions.  Tested a cross to x86_64-linux on the PR65693 testcase,
and it fixes it.

Is this okay for current trunk?


Segher


2015-04-08  Segher Boessenkool  <segher@kernel.crashing.org>

	* combine.c (is_parallel_of_n_reg_sets): Change first argument
	from an rtx_insn * to an rtx.
	(try_combine): Adjust both callers.  Use it once more.

---
 gcc/combine.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/gcc/combine.c b/gcc/combine.c
index 14df228..32950383 100644
--- a/gcc/combine.c
+++ b/gcc/combine.c
@@ -2493,13 +2493,11 @@ update_cfg_for_uncondjump (rtx_insn *insn)
 }
 
 #ifndef HAVE_cc0
-/* Return whether INSN is a PARALLEL of exactly N register SETs followed
+/* Return whether PAT is a PARALLEL of exactly N register SETs followed
    by an arbitrary number of CLOBBERs.  */
 static bool
-is_parallel_of_n_reg_sets (rtx_insn *insn, int n)
+is_parallel_of_n_reg_sets (rtx pat, int n)
 {
-  rtx pat = PATTERN (insn);
-
   if (GET_CODE (pat) != PARALLEL)
     return false;
 
@@ -2907,7 +2905,7 @@ try_combine (rtx_insn *i3, rtx_insn *i2, rtx_insn *i1, rtx_insn *i0,
      decrement insn.  */
 
   if (i1 == 0
-      && is_parallel_of_n_reg_sets (i2, 2)
+      && is_parallel_of_n_reg_sets (PATTERN (i2), 2)
       && (GET_MODE_CLASS (GET_MODE (SET_DEST (XVECEXP (PATTERN (i2), 0, 0))))
 	  == MODE_CC)
       && GET_CODE (SET_SRC (XVECEXP (PATTERN (i2), 0, 0))) == COMPARE
@@ -2939,7 +2937,7 @@ try_combine (rtx_insn *i3, rtx_insn *i2, rtx_insn *i1, rtx_insn *i0,
      make those two SETs separate I1 and I2 insns, and make an I0 that is
      the original I1.  */
   if (i0 == 0
-      && is_parallel_of_n_reg_sets (i2, 2)
+      && is_parallel_of_n_reg_sets (PATTERN (i2), 2)
       && can_split_parallel_of_n_reg_sets (i2, 2)
       && !reg_used_between_p (SET_DEST (XVECEXP (PATTERN (i2), 0, 0)), i2, i3)
       && !reg_used_between_p (SET_DEST (XVECEXP (PATTERN (i2), 0, 1)), i2, i3))
@@ -3460,10 +3458,7 @@ try_combine (rtx_insn *i3, rtx_insn *i2, rtx_insn *i1, rtx_insn *i0,
      debug info less accurate.  */
 
   if (!(added_sets_2 && i1 == 0)
-      && GET_CODE (newpat) == PARALLEL
-      && XVECLEN (newpat, 0) == 2
-      && GET_CODE (XVECEXP (newpat, 0, 0)) == SET
-      && GET_CODE (XVECEXP (newpat, 0, 1)) == SET
+      && is_parallel_of_n_reg_sets (newpat, 2)
       && asm_noperands (newpat) < 0)
     {
       rtx set0 = XVECEXP (newpat, 0, 0);
-- 
1.8.1.4

Re: [PATCH] combine: Disregard clobbers in another test for two SETs (PR65693)

[Jeff Law]

On 04/08/2015 03:33 PM, Segher Boessenkool wrote:
> PR65693 exposes a case where combine does a worse job after my patches
> to split parallels before combining.  We start with a parallel of an
> udiv and an umod, and a clobber; the umod is dead.  The instruction
> is combined with one setting the divisor pseudo to a power-of-two
> constant, so we end up with a parallel of an lshiftrt, an and (dead),
> and a clobber.  This is not a recognised instruction.
>
> Before my patches this was a 2->1 combination, and the combiner throws
> away the dead set and everyone is happy.  After the patches, this now
> is a 3->1 combination, the combiner does not throw away the dead set
> but tries to split the parallel into two, which does not work because
> one of the resulting insns has to end up as i2, which is earlier than
> the original sets.  The combiner gives up.
>
> There already is code to throw away dead sets in the 3->1 case, but
> it only works for a parallel for two sets without any clobbers.  This
> patch fixes it.
>
> Tested on powerpc64-linux (-m32,-m32/-mpowerpc64,-m64,-m64/-mlra);
> no regressions.  Tested a cross to x86_64-linux on the PR65693 testcase,
> and it fixes it.
>
> Is this okay for current trunk?
>
>
> Segher
>
>
> 2015-04-08  Segher Boessenkool  <segher@kernel.crashing.org>
>
> 	* combine.c (is_parallel_of_n_reg_sets): Change first argument
> 	from an rtx_insn * to an rtx.
> 	(try_combine): Adjust both callers.  Use it once more.
OK.
jeff

breakage with "[PATCH] combine: Disregard clobbers in another test for two SETs (PR65693)"

[Hans-Peter Nilsson]

On Wed, 8 Apr 2015, Segher Boessenkool wrote:
> 2015-04-08  Segher Boessenkool  <segher@kernel.crashing.org>
>
> 	* combine.c (is_parallel_of_n_reg_sets): Change first argument
> 	from an rtx_insn * to an rtx.
> 	(try_combine): Adjust both callers.  Use it once more.

That "once more" is outside of #ifndef HAVE_cc0 and
is_parallel_of_n_reg_sets is only defined inside of one.  Boom.
Full test on a cc0 target (such as cris-elf) is advised, and at
least "make all-gcc" would be a minimum after fixing.

cutnpaste:

g++ -c   -g -O2 -DIN_GCC  -DCROSS_DIRECTORY_STRUCTURE
-fno-exceptions -fno-rtti -fasynchronous-unwind-tables -W -Wall
\
-Wwrite-strings -Wcast-qual -Wmissing-format-attribute
-Woverloaded-virtual -pedantic -Wno-long-long
-Wno-variadic-macr\
os -Wno-overlength-strings -fno-common  -DHAVE_CONFIG_H -I. -I.
-I/tmp/hpautotest-gcc1/gcc/gcc -I/tmp/hpautotest-gcc1/g\
cc/gcc/. -I/tmp/hpautotest-gcc1/gcc/gcc/../include
-I/tmp/hpautotest-gcc1/gcc/gcc/../libcpp/include
-I/tmp/hpautotest-g\
cc1/cris-elf/gccobj/./gmp -I/tmp/hpautotest-gcc1/gcc/gmp
-I/tmp/hpautotest-gcc1/cris-elf/gccobj/./mpfr -I/tmp/hpautotes\
t-gcc1/gcc/mpfr -I/tmp/hpautotest-gcc1/gcc/mpc/src
-I/tmp/hpautotest-gcc1/gcc/gcc/../libdecnumber
-I/tmp/hpautotest-gc\
c1/gcc/gcc/../libdecnumber/dpd -I../libdecnumber
-I/tmp/hpautotest-gcc1/gcc/gcc/../libbacktrace   -o combine.o
-MT comb\
ine.o -MMD -MP -MF ./.deps/combine.TPo
/tmp/hpautotest-gcc1/gcc/gcc/combine.c
/tmp/hpautotest-gcc1/gcc/gcc/combine.c: In function 'rtx_insn*
try_combine(rtx_insn*, rtx_insn*, rtx_insn*, rtx_insn*, \
int*, rtx_insn*)':
/tmp/hpautotest-gcc1/gcc/gcc/combine.c:3461: error:
'is_parallel_of_n_reg_sets' was not declared in this scope
make[2]: *** [combine.o] Error 1

brgds, H-P

Re: breakage with "[PATCH] combine: Disregard clobbers in another test for two SETs (PR65693)"

[Segher Boessenkool]

On Wed, Apr 08, 2015 at 10:29:03PM -0400, Hans-Peter Nilsson wrote:
> On Wed, 8 Apr 2015, Segher Boessenkool wrote:
> > 2015-04-08  Segher Boessenkool  <segher@kernel.crashing.org>
> >
> > 	* combine.c (is_parallel_of_n_reg_sets): Change first argument
> > 	from an rtx_insn * to an rtx.
> > 	(try_combine): Adjust both callers.  Use it once more.

Hi,

> That "once more" is outside of #ifndef HAVE_cc0 and
> is_parallel_of_n_reg_sets is only defined inside of one.  Boom.

Oops.  So sorry.

> Full test on a cc0 target (such as cris-elf) is advised, and at
> least "make all-gcc" would be a minimum after fixing.

I tested a cross to cris-linux and built a Linux kernel with the trivial
patch (attached); doing that for all other cross configs is in progress.

It would be nice if there would be some cc0 target in the compile farm,
since cc0 isn't going away any time soon :-(

Okay if testing has finished successfully?


Segher


2015-04-09  Segher Boessenkool  <segher@kernel.crashing.org>

	PR rtl-optimization/65693
	* combine.c (is_parallel_of_n_reg_sets): Move outside of
	#ifndef HAVE_cc0.

---
 gcc/combine.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gcc/combine.c b/gcc/combine.c
index 32950383..0836f74 100644
--- a/gcc/combine.c
+++ b/gcc/combine.c
@@ -2492,7 +2492,6 @@ update_cfg_for_uncondjump (rtx_insn *insn)
     }
 }
 
-#ifndef HAVE_cc0
 /* Return whether PAT is a PARALLEL of exactly N register SETs followed
    by an arbitrary number of CLOBBERs.  */
 static bool
@@ -2517,6 +2516,7 @@ is_parallel_of_n_reg_sets (rtx pat, int n)
   return true;
 }
 
+#ifndef HAVE_cc0
 /* Return whether INSN, a PARALLEL of N register SETs (and maybe some
    CLOBBERs), can be split into individual SETs in that order, without
    changing semantics.  */
-- 
1.8.1.4

Re: breakage with "[PATCH] combine: Disregard clobbers in another test for two SETs (PR65693)"

[Jakub Jelinek]

On Thu, Apr 09, 2015 at 07:41:17AM -0500, Segher Boessenkool wrote:
> It would be nice if there would be some cc0 target in the compile farm,
> since cc0 isn't going away any time soon :-(
> 
> Okay if testing has finished successfully?
> 
> 
> Segher
> 
> 
> 2015-04-09  Segher Boessenkool  <segher@kernel.crashing.org>
> 
> 	PR rtl-optimization/65693
> 	* combine.c (is_parallel_of_n_reg_sets): Move outside of
> 	#ifndef HAVE_cc0.

This is obviously ok.

	Jakub

Re: breakage with "[PATCH] combine: Disregard clobbers in another test for two SETs (PR65693)"

[Steven Bosscher]

On Thu, Apr 9, 2015 at 2:41 PM, Segher Boessenkool wrote:
> It would be nice if there would be some cc0 target in the compile farm,
> since cc0 isn't going away any time soon :-(

In this case it would be enough to replace the "#ifndef/#ifdef
HAVE_cc0" code with "if (HAVE_cc0)".

That's the simplest way to avoid compile breakage. Likewise for so
many other #ifdef code (HAVE_conditional_move, HAVE_trap, etc.).

Perhaps something to work on in the next stage1...

Ciao!
Steven

Re: breakage with "[PATCH] combine: Disregard clobbers in another test for two SETs (PR65693)"

[Jeff Law]

On 04/09/2015 07:21 AM, Steven Bosscher wrote:
> On Thu, Apr 9, 2015 at 2:41 PM, Segher Boessenkool wrote:
>> It would be nice if there would be some cc0 target in the compile farm,
>> since cc0 isn't going away any time soon :-(
>
> In this case it would be enough to replace the "#ifndef/#ifdef
> HAVE_cc0" code with "if (HAVE_cc0)".
>
> That's the simplest way to avoid compile breakage. Likewise for so
> many other #ifdef code (HAVE_conditional_move, HAVE_trap, etc.).
>
> Perhaps something to work on in the next stage1...
Most definitely a direction I want to see us moving.  The glibc project 
recently went through the pain of the transition, but I believe it'll be 
worth it long term for them and even more so for us (we have a lot more 
conditionally compiled code than glibc).

jeff

Re: breakage with "[PATCH] combine: Disregard clobbers in another test for two SETs (PR65693)"

[Segher Boessenkool]

On Thu, Apr 09, 2015 at 03:21:44PM +0200, Steven Bosscher wrote:
> On Thu, Apr 9, 2015 at 2:41 PM, Segher Boessenkool wrote:
> > It would be nice if there would be some cc0 target in the compile farm,
> > since cc0 isn't going away any time soon :-(
> 
> In this case it would be enough to replace the "#ifndef/#ifdef
> HAVE_cc0" code with "if (HAVE_cc0)".
> 
> That's the simplest way to avoid compile breakage. Likewise for so
> many other #ifdef code (HAVE_conditional_move, HAVE_trap, etc.).

If the code inside the #ifdef can actually compile for the opposite
condition, yeah.

The bad effect of not breaking compilation for cc0 targets is we are
even less likely to consider whether something would break on cc0 ;-)

> Perhaps something to work on in the next stage1...

Thanks for volunteering!


Segher

Re: breakage with "[PATCH] combine: Disregard clobbers in another test for two SETs (PR65693)"

[Hans-Peter Nilsson]

On Thu, 9 Apr 2015, Segher Boessenkool wrote:
> I tested a cross to cris-linux and built a Linux kernel with the trivial
> patch (attached); doing that for all other cross configs is in progress.

Thanks.  Using contrib/config-list.mk comes to mind, but might
be a bit excessive in this particular case.

> It would be nice if there would be some cc0 target in the compile farm,
> since cc0 isn't going away any time soon :-(

Canned reply: use simulator targets,
<http://gcc.gnu.org/simtest-howto.html>.

brgds, H-P