[PATCH] combine: Allow substituting the target reg of a clobber

[PATCH] combine: Allow substituting the target reg of a clobber

[Segher Boessenkool]

This came up when investigating PR62151.  In that PR combine messes up a
four-insn combination.  It should really have done the combination of the
first three insns in that.  The second of those instructions sets a register;
the third clobbers the same.  Substituting the source of the set into the
clobber usually results in invalid RTL so that the combination will not be
valid; also, it confuses the undobuf machinery.  can_combine_p rejects
the combination for this reason.

But we can simply make subst not substitute into clobbers of registers.
Any unnecessary clobbers will be removed later anyway.

With this patch, the three-insn combination in PR62151 is successful.  It
does likely not really solve the problem there though, it just hides it.

Bootstrapped and regression checked on powerpc64-linux, options
-m64,-m32,-m32/-mpowerpc64.

Is this okay for mainline?


Segher


2014-09-03  Segher Boessenkool  <segher@kernel.crashing.org>

	PR rtl-optimization/62151
	* combine.c (can_combine_p): Allow the destination register of INSN
	to be clobbered in I3.
	(subst): Do not substitute into clobbers of registers.

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

diff --git a/gcc/combine.c b/gcc/combine.c
index 60524b5..6a5dfbb 100644
--- a/gcc/combine.c
+++ b/gcc/combine.c
@@ -1950,11 +1950,7 @@ can_combine_p (rtx_insn *insn, rtx_insn *i3, rtx_insn *pred ATTRIBUTE_UNUSED,
     for (i = XVECLEN (PATTERN (i3), 0) - 1; i >= 0; i--)
       if (GET_CODE (XVECEXP (PATTERN (i3), 0, i)) == CLOBBER)
 	{
-	  /* Don't substitute for a register intended as a clobberable
-	     operand.  */
 	  rtx reg = XEXP (XVECEXP (PATTERN (i3), 0, i), 0);
-	  if (rtx_equal_p (reg, dest))
-	    return 0;
 
 	  /* If the clobber represents an earlyclobber operand, we must not
 	     substitute an expression containing the clobbered register.
@@ -4964,6 +4960,11 @@ subst (rtx x, rtx from, rtx to, int in_dest, int in_cond, int unique_copy)
    || (REG_P (X) && REG_P (Y)	\
        && REGNO (X) == REGNO (Y) && GET_MODE (X) == GET_MODE (Y)))
 
+  /* Do not substitute into clobbers of regs -- this will never result in
+     valid RTL.  */
+  if (GET_CODE (x) == CLOBBER && REG_P (XEXP (x, 0)))
+    return x;
+
   if (! in_dest && COMBINE_RTX_EQUAL_P (x, from))
     {
       n_occurrences++;
-- 
1.8.1.4

Re: [PATCH] combine: Allow substituting the target reg of a clobber

[Segher Boessenkool]

Ping?

On Wed, Sep 03, 2014 at 06:51:44AM -0700, Segher Boessenkool wrote:
> This came up when investigating PR62151.  In that PR combine messes up a
> four-insn combination.  It should really have done the combination of the
> first three insns in that.  The second of those instructions sets a register;
> the third clobbers the same.  Substituting the source of the set into the
> clobber usually results in invalid RTL so that the combination will not be
> valid; also, it confuses the undobuf machinery.  can_combine_p rejects
> the combination for this reason.
> 
> But we can simply make subst not substitute into clobbers of registers.
> Any unnecessary clobbers will be removed later anyway.
> 
> With this patch, the three-insn combination in PR62151 is successful.  It
> does likely not really solve the problem there though, it just hides it.
> 
> Bootstrapped and regression checked on powerpc64-linux, options
> -m64,-m32,-m32/-mpowerpc64.
> 
> Is this okay for mainline?
> 
> 
> Segher
> 
> 
> 2014-09-03  Segher Boessenkool  <segher@kernel.crashing.org>
> 
> 	PR rtl-optimization/62151
> 	* combine.c (can_combine_p): Allow the destination register of INSN
> 	to be clobbered in I3.
> 	(subst): Do not substitute into clobbers of registers.
> 
> ---
>  gcc/combine.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/gcc/combine.c b/gcc/combine.c
> index 60524b5..6a5dfbb 100644
> --- a/gcc/combine.c
> +++ b/gcc/combine.c
> @@ -1950,11 +1950,7 @@ can_combine_p (rtx_insn *insn, rtx_insn *i3, rtx_insn *pred ATTRIBUTE_UNUSED,
>      for (i = XVECLEN (PATTERN (i3), 0) - 1; i >= 0; i--)
>        if (GET_CODE (XVECEXP (PATTERN (i3), 0, i)) == CLOBBER)
>  	{
> -	  /* Don't substitute for a register intended as a clobberable
> -	     operand.  */
>  	  rtx reg = XEXP (XVECEXP (PATTERN (i3), 0, i), 0);
> -	  if (rtx_equal_p (reg, dest))
> -	    return 0;
>  
>  	  /* If the clobber represents an earlyclobber operand, we must not
>  	     substitute an expression containing the clobbered register.
> @@ -4964,6 +4960,11 @@ subst (rtx x, rtx from, rtx to, int in_dest, int in_cond, int unique_copy)
>     || (REG_P (X) && REG_P (Y)	\
>         && REGNO (X) == REGNO (Y) && GET_MODE (X) == GET_MODE (Y)))
>  
> +  /* Do not substitute into clobbers of regs -- this will never result in
> +     valid RTL.  */
> +  if (GET_CODE (x) == CLOBBER && REG_P (XEXP (x, 0)))
> +    return x;
> +
>    if (! in_dest && COMBINE_RTX_EQUAL_P (x, from))
>      {
>        n_occurrences++;
> -- 
> 1.8.1.4

Re: [PATCH] combine: Allow substituting the target reg of a clobber

[Jeff Law]

On 09/11/14 13:01, Segher Boessenkool wrote:
> Ping?
Not forgotten.  Still waiting to hear back from Bin on my recommendation 
that we drop the bogus note on the floor and avoid combining pseudos 
with multiple sets like that.

Jeff

Re: [PATCH] combine: Allow substituting the target reg of a clobber

[Jeff Law]

On 09/03/14 07:51, Segher Boessenkool wrote:
> This came up when investigating PR62151.  In that PR combine messes up a
> four-insn combination.  It should really have done the combination of the
> first three insns in that.  The second of those instructions sets a register;
> the third clobbers the same.  Substituting the source of the set into the
> clobber usually results in invalid RTL so that the combination will not be
> valid; also, it confuses the undobuf machinery.  can_combine_p rejects
> the combination for this reason.
>
> But we can simply make subst not substitute into clobbers of registers.
> Any unnecessary clobbers will be removed later anyway.
>
> With this patch, the three-insn combination in PR62151 is successful.  It
> does likely not really solve the problem there though, it just hides it.
>
> Bootstrapped and regression checked on powerpc64-linux, options
> -m64,-m32,-m32/-mpowerpc64.
>
> Is this okay for mainline?
>
>
> Segher
>
>
> 2014-09-03  Segher Boessenkool  <segher@kernel.crashing.org>
>
> 	PR rtl-optimization/62151
> 	* combine.c (can_combine_p): Allow the destination register of INSN
> 	to be clobbered in I3.
> 	(subst): Do not substitute into clobbers of registers.
Can you add a testcase which shows the 3-insn combination from PR62151 
applying?

With that change, approved.

jeff

Re: [PATCH] combine: Allow substituting the target reg of a clobber

[Bin.Cheng]

On Fri, Sep 12, 2014 at 4:01 AM, Jeff Law <law@redhat.com> wrote:
> On 09/11/14 13:01, Segher Boessenkool wrote:
>>
>> Ping?
>
> Not forgotten.  Still waiting to hear back from Bin on my recommendation
> that we drop the bogus note on the floor and avoid combining pseudos with
> multiple sets like that.

Sorry that I missed this message days ago.  I am currently on
load/store pair which has higher priority.  I will re-visit this after
load/store pair issue, only thing I need is to include test case from
PR62151 thus I can keep track of it, just as Jeff commented.

Thanks,
bin

Re: [PATCH] combine: Allow substituting the target reg of a clobber

[Segher Boessenkool]

On Mon, Sep 22, 2014 at 04:20:12PM -0600, Jeff Law wrote:
> Can you add a testcase which shows the 3-insn combination from PR62151 
> applying?

I've tried to make a stable future-proof testcase that does such a three-insn
combination.  Not easy at all.

But now it dawns on me: do you just want the actual testcase from the PR?
(Well, fixed so that it is valid C, I suppose).  With a test that combine
does its job, of course?  Not sure how to test that, but maybe I'll learn.

Or is a test showing the testcase working after the change good enough?


Segher

Re: [PATCH] combine: Allow substituting the target reg of a clobber

[Jeff Law]

On 09/27/14 16:03, Segher Boessenkool wrote:
> On Mon, Sep 22, 2014 at 04:20:12PM -0600, Jeff Law wrote:
>> Can you add a testcase which shows the 3-insn combination from PR62151
>> applying?
>
> I've tried to make a stable future-proof testcase that does such a three-insn
> combination.  Not easy at all.
>
> But now it dawns on me: do you just want the actual testcase from the PR?
> (Well, fixed so that it is valid C, I suppose).  With a test that combine
> does its job, of course?  Not sure how to test that, but maybe I'll learn.
>
> Or is a test showing the testcase working after the change good enough?
It's going to be hard to totally future proof this kind of test and make 
it independent across every target, etc.

In those cases, I just ask you do something reasonable.   That can 
include making the test only applicable on a small number of targets and 
testing the RTL or assembly dumps.

So, I'd use the testcase from the PR and probably scan the combine dump 
and probably make it target dependent.  If the combine dump isn't 
particularly good to search for some reason, scan the assembly output.

jeff

Re: [PATCH] combine: Allow substituting the target reg of a clobber

[Segher Boessenkool]

On Sat, Sep 27, 2014 at 05:03:26PM -0500, Segher Boessenkool wrote:
> I've tried to make a stable future-proof testcase that does such a three-insn
> combination.  Not easy at all.

Turns out it is quite easy (when you've seen the solution, anyway :-P )

Tested on powerpc64-linux as before, and bootstrapped + regression checked
on x86_64-linux.  Also checked the testcase fails before on i386 and x86_64,
and works afterwards.

Does this look good?


Segher



2014-10-02  Segher Boessenkool  <segher@kernel.crashing.org>

gcc/
	PR rtl-optimization/62151
	* combine.c (can_combine_p): Allow the destination register of INSN
	to be clobbered in I3.
	(subst): Do not substitute into clobbers of registers.

gcc/testsuite/
	* gcc.dg/combine-clobber.c: New.



diff --git a/gcc/combine.c b/gcc/combine.c
index 1457eab..ff5f0db 100644
--- a/gcc/combine.c
+++ b/gcc/combine.c
@@ -1950,11 +1950,7 @@ can_combine_p (rtx_insn *insn, rtx_insn *i3, rtx_insn *pred ATTRIBUTE_UNUSED,
     for (i = XVECLEN (PATTERN (i3), 0) - 1; i >= 0; i--)
       if (GET_CODE (XVECEXP (PATTERN (i3), 0, i)) == CLOBBER)
 	{
-	  /* Don't substitute for a register intended as a clobberable
-	     operand.  */
 	  rtx reg = XEXP (XVECEXP (PATTERN (i3), 0, i), 0);
-	  if (rtx_equal_p (reg, dest))
-	    return 0;
 
 	  /* If the clobber represents an earlyclobber operand, we must not
 	     substitute an expression containing the clobbered register.
@@ -4963,6 +4959,11 @@ subst (rtx x, rtx from, rtx to, int in_dest, int in_cond, int unique_copy)
    || (REG_P (X) && REG_P (Y)	\
        && REGNO (X) == REGNO (Y) && GET_MODE (X) == GET_MODE (Y)))
 
+  /* Do not substitute into clobbers of regs -- this will never result in
+     valid RTL.  */
+  if (GET_CODE (x) == CLOBBER && REG_P (XEXP (x, 0)))
+    return x;
+
   if (! in_dest && COMBINE_RTX_EQUAL_P (x, from))
     {
       n_occurrences++;
diff --git a/gcc/testsuite/gcc.dg/combine-clobber.c b/gcc/testsuite/gcc.dg/combine-clobber.c
new file mode 100644
index 0000000..bf0d88c
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/combine-clobber.c
@@ -0,0 +1,22 @@
+/* { dg-do compile { target i?86-*-* x86_64-*-* } } */
+/* { dg-options "-O2 -fdump-rtl-combine-all" } */
+
+/* This testcase checks if combine tries to combine sequences where the last
+   insn has a clobber of a reg, and a previous insn sets that reg.
+
+   In this case, we have three insns
+
+   (set flags (compare a b))
+   (set tmp (eq flags 0))
+   (parallel [(set dst (neg tmp))
+	      (clobber flags)])
+
+   Previously, combine would not try the three-insn combination because of
+   the set and clobber of flags.  Now it does.  Test that.  */
+
+
+int f(int a, int b) { return -(a == b); }
+
+/* This regexp works for reg parameters as well as mem parameters.  */
+/* { dg-final { scan-rtl-dump {neg:SI[^:]*eq:SI[^:]*:SI} "combine" } } */
+/* { dg-final { cleanup-rtl-dump "combine" } } */

Re: [PATCH] combine: Allow substituting the target reg of a clobber

[Jeff Law]

On 10/01/14 16:36, Segher Boessenkool wrote:
> On Sat, Sep 27, 2014 at 05:03:26PM -0500, Segher Boessenkool wrote:
>> I've tried to make a stable future-proof testcase that does such a three-insn
>> combination.  Not easy at all.
>
> Turns out it is quite easy (when you've seen the solution, anyway :-P )
>
> Tested on powerpc64-linux as before, and bootstrapped + regression checked
> on x86_64-linux.  Also checked the testcase fails before on i386 and x86_64,
> and works afterwards.
>
> Does this look good?
>
>
> Segher
>
>
>
> 2014-10-02  Segher Boessenkool  <segher@kernel.crashing.org>
>
> gcc/
> 	PR rtl-optimization/62151
> 	* combine.c (can_combine_p): Allow the destination register of INSN
> 	to be clobbered in I3.
> 	(subst): Do not substitute into clobbers of registers.
>
> gcc/testsuite/
> 	* gcc.dg/combine-clobber.c: New.
Excellent.  Thanks.  Even more so for making the test for x86 since 
that's the platform that gets tested the most often.

Ok for the trunk.

jeff