diff mbox

[v3] Do not simplify "(and (reg) (const bit))" to if_then_else.

Message ID 20161201153008.GA28115@linux.vnet.ibm.com
State New
Headers show

Commit Message

Dominik Vogt Dec. 1, 2016, 3:30 p.m. UTC 
On Thu, Dec 01, 2016 at 01:33:17PM +0100, Bernd Schmidt wrote:
> On 11/21/2016 01:36 PM, Dominik Vogt wrote:
> >diff --git a/gcc/combine.c b/gcc/combine.c
> >index b22a274..457fe8a 100644
> >--- a/gcc/combine.c
> >+++ b/gcc/combine.c
> >@@ -5575,10 +5575,23 @@ combine_simplify_rtx (rtx x, machine_mode op0_mode, int in_dest,
> > 	{
> > 	  rtx cop1 = const0_rtx;
> > 	  enum rtx_code cond_code = simplify_comparison (NE, &cond, &cop1);
> >+	  unsigned HOST_WIDE_INT nz;
> >
> > 	  if (cond_code == NE && COMPARISON_P (cond))
> > 	    return x;
> >
> >+	  /* If the operation is an AND wrapped in a SIGN_EXTEND or ZERO_EXTEND
> >+	     with either operand being just a constant single bit value, do
> >+	     nothing since IF_THEN_ELSE is likely to increase the expression's
> >+	     complexity.  */
> >+	  if (HWI_COMPUTABLE_MODE_P (mode)
> >+	      && pow2p_hwi (nz = nonzero_bits (x, mode))
> >+	      && ! ((code == SIGN_EXTEND || code == ZERO_EXTEND)
> >+		    && GET_CODE (XEXP (x, 0)) == AND
> >+		    && CONST_INT_P (XEXP (XEXP (x, 0), 0))
> >+		    && UINTVAL (XEXP (XEXP (x, 0), 0)) == nz))
> >+	    return x;
> 
> It looks like this doesn't actually use cond or true/false_rtx. So
> this could be placed just above the call to if_then_else_cond to
> avoid unnecessary work. Ok if that works.

It does.  Version 3 attached, bootstrapped on s390x and regression
tested on s390x biarch and s390.

Ciao

Dominik ^_^  ^_^

Comments

Andreas Krebbel Dec. 2, 2016, 8:36 a.m. UTC | #1 
On Thu, Dec 01, 2016 at 04:30:08PM +0100, Dominik Vogt wrote:
> gcc/ChangeLog
> 
> 	* combine.c (combine_simplify_rtx):  Suppress replacement of
> 	"(and (reg) (const_int bit))" with "if_then_else".

Applied. Thanks!

-Andreas-
Andreas Schwab Dec. 3, 2016, 4:35 p.m. UTC | #2 
On Dez 01 2016, Dominik Vogt <vogt@linux.vnet.ibm.com> wrote:

> gcc/ChangeLog
>
> 	* combine.c (combine_simplify_rtx):  Suppress replacement of
> 	"(and (reg) (const_int bit))" with "if_then_else".

That causes a regression on ia64.

FAIL: gcc.target/ia64/builtin-popcount-2.c scan-assembler-not popcnt

Andreas.
Segher Boessenkool Dec. 4, 2016, 1:19 a.m. UTC | #3 
[ I did not see this patch before, sorry. ]

This causes the second half of PR78638.

On Thu, Dec 01, 2016 at 04:30:08PM +0100, Dominik Vogt wrote:
> --- a/gcc/combine.c
> +++ b/gcc/combine.c
> @@ -5600,6 +5600,18 @@ combine_simplify_rtx (rtx x, machine_mode op0_mode, int in_dest,
>  		     && OBJECT_P (SUBREG_REG (XEXP (x, 0)))))))
>      {
>        rtx cond, true_rtx, false_rtx;
> +      unsigned HOST_WIDE_INT nz;
> +
> +      /* If the operation is an AND wrapped in a SIGN_EXTEND or ZERO_EXTEND with
> +	 either operand being just a constant single bit value, do nothing since
> +	 IF_THEN_ELSE is likely to increase the expression's complexity.  */
> +      if (HWI_COMPUTABLE_MODE_P (mode)
> +	  && pow2p_hwi (nz = nonzero_bits (x, mode))
> +	  && ! ((code == SIGN_EXTEND || code == ZERO_EXTEND)
> +		&& GET_CODE (XEXP (x, 0)) == AND
> +		&& CONST_INT_P (XEXP (XEXP (x, 0), 0))
> +		&& UINTVAL (XEXP (XEXP (x, 0), 0)) == nz))
> +	      return x;

The code does not match the comment: the "!" should not be there.  How
did it fix anything on s390 *with* that "!"?  That does not make much
sense.


Segher
Dominik Vogt Dec. 5, 2016, 9:22 a.m. UTC | #4 
On Sat, Dec 03, 2016 at 07:19:13PM -0600, Segher Boessenkool wrote:
> [ I did not see this patch before, sorry. ]
> 
> This causes the second half of PR78638.
> 
> On Thu, Dec 01, 2016 at 04:30:08PM +0100, Dominik Vogt wrote:
> > --- a/gcc/combine.c
> > +++ b/gcc/combine.c
> > @@ -5600,6 +5600,18 @@ combine_simplify_rtx (rtx x, machine_mode op0_mode, int in_dest,
> >  		     && OBJECT_P (SUBREG_REG (XEXP (x, 0)))))))
> >      {
> >        rtx cond, true_rtx, false_rtx;
> > +      unsigned HOST_WIDE_INT nz;
> > +
> > +      /* If the operation is an AND wrapped in a SIGN_EXTEND or ZERO_EXTEND with
> > +	 either operand being just a constant single bit value, do nothing since
> > +	 IF_THEN_ELSE is likely to increase the expression's complexity.  */
> > +      if (HWI_COMPUTABLE_MODE_P (mode)
> > +	  && pow2p_hwi (nz = nonzero_bits (x, mode))
> > +	  && ! ((code == SIGN_EXTEND || code == ZERO_EXTEND)
> > +		&& GET_CODE (XEXP (x, 0)) == AND
> > +		&& CONST_INT_P (XEXP (XEXP (x, 0), 0))
> > +		&& UINTVAL (XEXP (XEXP (x, 0), 0)) == nz))
> > +	      return x;
> 
> The code does not match the comment: the "!" should not be there.  How
> did it fix anything on s390 *with* that "!"?  That does not make much
> sense.

Sorry for breaking this.  With the constant changes in the
patterns this is supposed to fix it seems I've lost track of the
status quo.  I'll check what went wrong with the patch; in the
mean time Andreas will revert this, or if it's urgent, feel free
to do that yourself.

Ciao

Dominik ^_^  ^_^
Segher Boessenkool Dec. 5, 2016, 10 a.m. UTC | #5 
On Mon, Dec 05, 2016 at 10:22:13AM +0100, Dominik Vogt wrote:
> On Sat, Dec 03, 2016 at 07:19:13PM -0600, Segher Boessenkool wrote:
> > [ I did not see this patch before, sorry. ]
> > 
> > This causes the second half of PR78638.
> > 
> > On Thu, Dec 01, 2016 at 04:30:08PM +0100, Dominik Vogt wrote:
> > > --- a/gcc/combine.c
> > > +++ b/gcc/combine.c
> > > @@ -5600,6 +5600,18 @@ combine_simplify_rtx (rtx x, machine_mode op0_mode, int in_dest,
> > >  		     && OBJECT_P (SUBREG_REG (XEXP (x, 0)))))))
> > >      {
> > >        rtx cond, true_rtx, false_rtx;
> > > +      unsigned HOST_WIDE_INT nz;
> > > +
> > > +      /* If the operation is an AND wrapped in a SIGN_EXTEND or ZERO_EXTEND with
> > > +	 either operand being just a constant single bit value, do nothing since
> > > +	 IF_THEN_ELSE is likely to increase the expression's complexity.  */
> > > +      if (HWI_COMPUTABLE_MODE_P (mode)
> > > +	  && pow2p_hwi (nz = nonzero_bits (x, mode))
> > > +	  && ! ((code == SIGN_EXTEND || code == ZERO_EXTEND)
> > > +		&& GET_CODE (XEXP (x, 0)) == AND
> > > +		&& CONST_INT_P (XEXP (XEXP (x, 0), 0))
> > > +		&& UINTVAL (XEXP (XEXP (x, 0), 0)) == nz))
> > > +	      return x;
> > 
> > The code does not match the comment: the "!" should not be there.  How
> > did it fix anything on s390 *with* that "!"?  That does not make much
> > sense.
> 
> Sorry for breaking this.  With the constant changes in the
> patterns this is supposed to fix it seems I've lost track of the
> status quo.  I'll check what went wrong with the patch; in the
> mean time Andreas will revert this, or if it's urgent, feel free
> to do that yourself.

I have tested that removing that ! cures all regressions.  I do not
know if it still fixes what this patch intended to fix, of course.


Segher
Dominik Vogt Dec. 5, 2016, 10:49 a.m. UTC | #6 
On Mon, Dec 05, 2016 at 04:00:25AM -0600, Segher Boessenkool wrote:
> On Mon, Dec 05, 2016 at 10:22:13AM +0100, Dominik Vogt wrote:
> > On Sat, Dec 03, 2016 at 07:19:13PM -0600, Segher Boessenkool wrote:
> > > [ I did not see this patch before, sorry. ]
> > > 
> > > This causes the second half of PR78638.
> > > 
> > > On Thu, Dec 01, 2016 at 04:30:08PM +0100, Dominik Vogt wrote:
> > > > --- a/gcc/combine.c
> > > > +++ b/gcc/combine.c
> > > > @@ -5600,6 +5600,18 @@ combine_simplify_rtx (rtx x, machine_mode op0_mode, int in_dest,
> > > >  		     && OBJECT_P (SUBREG_REG (XEXP (x, 0)))))))
> > > >      {
> > > >        rtx cond, true_rtx, false_rtx;
> > > > +      unsigned HOST_WIDE_INT nz;
> > > > +
> > > > +      /* If the operation is an AND wrapped in a SIGN_EXTEND or ZERO_EXTEND with
> > > > +	 either operand being just a constant single bit value, do nothing since
> > > > +	 IF_THEN_ELSE is likely to increase the expression's complexity.  */
> > > > +      if (HWI_COMPUTABLE_MODE_P (mode)
> > > > +	  && pow2p_hwi (nz = nonzero_bits (x, mode))
> > > > +	  && ! ((code == SIGN_EXTEND || code == ZERO_EXTEND)
> > > > +		&& GET_CODE (XEXP (x, 0)) == AND
> > > > +		&& CONST_INT_P (XEXP (XEXP (x, 0), 0))
> > > > +		&& UINTVAL (XEXP (XEXP (x, 0), 0)) == nz))
> > > > +	      return x;
> > > 
> > > The code does not match the comment: the "!" should not be there.  How
> > > did it fix anything on s390 *with* that "!"?  That does not make much
> > > sense.
> > 
> > Sorry for breaking this.  With the constant changes in the
> > patterns this is supposed to fix it seems I've lost track of the
> > status quo.  I'll check what went wrong with the patch; in the
> > mean time Andreas will revert this, or if it's urgent, feel free
> > to do that yourself.
> 
> I have tested that removing that ! cures all regressions.  I do not
> know if it still fixes what this patch intended to fix, of course.

S390[x] has these complicated patterns for the risbg instruction,
and there's some ongoing work on patterns for related instructions
(rosbg, rxsbg) which needed the patch discussed here - at least at
some point in time.  But the risbg patterns are breaking all over
the place because they are so sensitive to changes in combine.c
(and possibly other passes), and any change fixing the old
patterns may affect the new ones.  In other words:  At the moment
I have no clue whether the discussed patch is still good for
anythin on s390[x].

If there was a consensus that the patch discussed here, with the
"!" fix is useful in any case, that would simplify my current
work, but

1) I've done no testing with it (only with the broken version of
   the patch),
2) it may be just a chunk of dead code.

Ciao

Dominik ^_^  ^_^
Oleg Endo Dec. 5, 2016, 1:30 p.m. UTC | #7 
On Mon, 2016-12-05 at 04:00 -0600, Segher Boessenkool wrote:
> On Mon, Dec 05, 2016 at 10:22:13AM +0100, Dominik Vogt wrote:
> > 
> > On Sat, Dec 03, 2016 at 07:19:13PM -0600, Segher Boessenkool wrote:
> > > 
> > > [ I did not see this patch before, sorry. ]
> > > 
> > > This causes the second half of PR78638.
> > > 
> > > On Thu, Dec 01, 2016 at 04:30:08PM +0100, Dominik Vogt wrote:
> > > > 
> > > > --- a/gcc/combine.c
> > > > +++ b/gcc/combine.c
> > > > @@ -5600,6 +5600,18 @@ combine_simplify_rtx (rtx x,
> > > > machine_mode op0_mode, int in_dest,
> > > >  		     && OBJECT_P (SUBREG_REG (XEXP (x,
> > > > 0)))))))
> > > >      {
> > > >        rtx cond, true_rtx, false_rtx;
> > > > +      unsigned HOST_WIDE_INT nz;
> > > > +
> > > > +      /* If the operation is an AND wrapped in a SIGN_EXTEND
> > > > or ZERO_EXTEND with
> > > > +	 either operand being just a constant single bit
> > > > value, do nothing since
> > > > +	 IF_THEN_ELSE is likely to increase the expression's
> > > > complexity.  */
> > > > +      if (HWI_COMPUTABLE_MODE_P (mode)
> > > > +	  && pow2p_hwi (nz = nonzero_bits (x, mode))
> > > > +	  && ! ((code == SIGN_EXTEND || code == ZERO_EXTEND)
> > > > +		&& GET_CODE (XEXP (x, 0)) == AND
> > > > +		&& CONST_INT_P (XEXP (XEXP (x, 0), 0))
> > > > +		&& UINTVAL (XEXP (XEXP (x, 0), 0)) == nz))
> > > > +	      return x;
> > > The code does not match the comment: the "!" should not be
> > > there.  How
> > > did it fix anything on s390 *with* that "!"?  That does not make
> > > much
> > > sense.
> > Sorry for breaking this.  With the constant changes in the
> > patterns this is supposed to fix it seems I've lost track of the
> > status quo.  I'll check what went wrong with the patch; in the
> > mean time Andreas will revert this, or if it's urgent, feel free
> > to do that yourself.

> I have tested that removing that ! cures all regressions.  I do not
> know if it still fixes what this patch intended to fix, of course.

I haven't been following this, but it seems some of these changes also
triggered bleh on SH:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78633

Cheers,
Oleg
Segher Boessenkool Dec. 5, 2016, 1:56 p.m. UTC | #8 
On Mon, Dec 05, 2016 at 10:22:13AM +0100, Dominik Vogt wrote:
> Sorry for breaking this.  With the constant changes in the
> patterns this is supposed to fix it seems I've lost track of the
> status quo.  I'll check what went wrong with the patch; in the
> mean time Andreas will revert this, or if it's urgent, feel free
> to do that yourself.

I've reverted it now, r243256.

Thanks,


Segher
Dominik Vogt Dec. 5, 2016, 2 p.m. UTC | #9 
On Mon, Dec 05, 2016 at 07:56:46AM -0600, Segher Boessenkool wrote:
> On Mon, Dec 05, 2016 at 10:22:13AM +0100, Dominik Vogt wrote:
> > Sorry for breaking this.  With the constant changes in the
> > patterns this is supposed to fix it seems I've lost track of the
> > status quo.  I'll check what went wrong with the patch; in the
> > mean time Andreas will revert this, or if it's urgent, feel free
> > to do that yourself.
> 
> I've reverted it now, r243256.

Thanks.  I need to think about this patch and the patch that is
based on it for a while, so there's no need to get the fixed patch
into svn for now.

Ciao

Dominik ^_^  ^_^
diff mbox

Patch

diff --git a/gcc/combine.c b/gcc/combine.c
index a8dae89..52bde9e 100644
--- a/gcc/combine.c
+++ b/gcc/combine.c
@@ -5600,6 +5600,18 @@  combine_simplify_rtx (rtx x, machine_mode op0_mode, int in_dest,
 		     && OBJECT_P (SUBREG_REG (XEXP (x, 0)))))))
     {
       rtx cond, true_rtx, false_rtx;
+      unsigned HOST_WIDE_INT nz;
+
+      /* If the operation is an AND wrapped in a SIGN_EXTEND or ZERO_EXTEND with
+	 either operand being just a constant single bit value, do nothing since
+	 IF_THEN_ELSE is likely to increase the expression's complexity.  */
+      if (HWI_COMPUTABLE_MODE_P (mode)
+	  && pow2p_hwi (nz = nonzero_bits (x, mode))
+	  && ! ((code == SIGN_EXTEND || code == ZERO_EXTEND)
+		&& GET_CODE (XEXP (x, 0)) == AND
+		&& CONST_INT_P (XEXP (XEXP (x, 0), 0))
+		&& UINTVAL (XEXP (XEXP (x, 0), 0)) == nz))
+	      return x;
 
       cond = if_then_else_cond (x, &true_rtx, &false_rtx);
       if (cond != 0