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

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

[Dominik Vogt]

[-- Attachment #1: Type: text/plain, Size: 968 bytes --]

The attached patch does a little change in
combine.c:combine_simplify_rtx() to prevent a "simplification"
where the rtl code gets more complex in reality.  The complete
description of the change can be found in the commit comment in
the attached patch.

The patch reduces the number of patterns in the s390 backend and
slightly reduces the size of the compiled SPEC2006 code.  (Code
size or runtime only tested on s390x with -m64.)  It is
theoretically possible that this patch leads to somewhat worse
code on some target if that only has a pattern for the formerly replaced
rtl expression but not for the original one.

The patch has passed the testsuite on s390, s390x biarch, x86_64
and Power biarch.

--

(I'm not sure whether the const_int expression can appear in both
operands or only as the second.  If the latter is the case, the
conditions can be simplified a bit.)

What do you think about this patch?

Ciao

Dominik ^_^  ^_^

-- 

Dominik Vogt
IBM Germany

[-- Attachment #2: 0001-ChangeLog --]
[-- Type: text/plain, Size: 125 bytes --]

gcc/ChangeLog

	* combine.c (if_then_else_cond): Suppress replacement of
	"(and (reg) (const_int bit))" with "if_then_else".

[-- Attachment #3: 0001-Do-not-simplify-and-reg-const-bit-to-if_then_else.patch --]
[-- Type: text/plain, Size: 2145 bytes --]

From bfa15721c760fa4cd9003050c3137cba3165139f Mon Sep 17 00:00:00 2001
From: Dominik Vogt <vogt@linux.vnet.ibm.com>
Date: Mon, 31 Oct 2016 09:00:31 +0100
Subject: [PATCH] Do not simplify "(and (reg) (const bit))" to if_then_else.

combine_simplify_rtx() tries to replace rtx expressions with just two
possible values with an experession that uses if_then_else:

  (if_then_else (condition) (value1) (value2))

If the original expression is e.g.

  (and (reg) (const_int 2))

where the constant is the mask for a single bit, the replacement results
in a more complex expression than before:

  (if_then_else (ne (zero_extract (reg) (1) (31))) (2) (0))

Similar replacements are done for

  (signextend (and ...))
  (zeroextend (and ...))

Suppress the replacement this special case in if_then_else_cond().
---
 gcc/combine.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/gcc/combine.c b/gcc/combine.c
index b22a274..244669d 100644
--- a/gcc/combine.c
+++ b/gcc/combine.c
@@ -9103,9 +9103,25 @@ if_then_else_cond (rtx x, rtx *ptrue, rtx *pfalse)
       return x;
     }
 
-  /* Likewise for 0 or a single bit.  */
+  /* Likewise for 0 or a single bit.
+     If the operation is an AND (possibly 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.  */
   else if (HWI_COMPUTABLE_MODE_P (mode)
-	   && pow2p_hwi (nz = nonzero_bits (x, mode)))
+	   && pow2p_hwi (nz = nonzero_bits (x, mode))
+	   && ! (code == AND
+		 && ((CONST_INT_P (XEXP (x, 0))
+		      && UINTVAL (XEXP (x, 0)) == nz)
+		     || (CONST_INT_P (XEXP (x, 1))
+			 && UINTVAL (XEXP (x, 1)) == nz)))
+	   && ! ((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)
+		     || (CONST_INT_P (XEXP (XEXP (x, 0), 1))
+			 && UINTVAL (XEXP (XEXP (x, 0), 1)) == nz)))
+	  )
     {
       *ptrue = gen_int_mode (nz, mode), *pfalse = const0_rtx;
       return x;
-- 
2.3.0

[PING, PATCH] Do not simplify "(and (reg) (const bit))" to if_then_else.

[Dominik Vogt]

Ping.

https://gcc.gnu.org/ml/gcc-patches/2016-10/msg02525.html

On Mon, Oct 31, 2016 at 08:56:10PM +0100, Dominik Vogt wrote:
> The attached patch does a little change in
> combine.c:combine_simplify_rtx() to prevent a "simplification"
> where the rtl code gets more complex in reality.  The complete
> description of the change can be found in the commit comment in
> the attached patch.
> 
> The patch reduces the number of patterns in the s390 backend and
> slightly reduces the size of the compiled SPEC2006 code.  (Code
> size or runtime only tested on s390x with -m64.)  It is
> theoretically possible that this patch leads to somewhat worse
> code on some target if that only has a pattern for the formerly replaced
> rtl expression but not for the original one.
> 
> The patch has passed the testsuite on s390, s390x biarch, x86_64
> and Power biarch.
> 
> --
> 
> (I'm not sure whether the const_int expression can appear in both
> operands or only as the second.  If the latter is the case, the
> conditions can be simplified a bit.)
> 
> What do you think about this patch?

Ciao

Dominik ^_^  ^_^

-- 

Dominik Vogt
IBM Germany

Re: [PATCH] Do not simplify "(and (reg) (const bit))" to if_then_else.

[Bernd Schmidt]

On 10/31/2016 08:56 PM, Dominik Vogt wrote:

> combine_simplify_rtx() tries to replace rtx expressions with just two
> possible values with an experession that uses if_then_else:
>
>   (if_then_else (condition) (value1) (value2))
>
> If the original expression is e.g.
>
>   (and (reg) (const_int 2))

I'm not convinced that if_then_else_cond is the right place to do this. 
That function is designed to answer the question of whether an rtx has 
exactly one of two values and under which condition; I feel it should 
continue to work this way.

Maybe simplify_ternary_expression needs to be taught to deal with this case?


Bernd

Re: [PATCH] Do not simplify "(and (reg) (const bit))" to if_then_else.

[Dominik Vogt]

On Mon, Nov 07, 2016 at 09:29:26PM +0100, Bernd Schmidt wrote:
> On 10/31/2016 08:56 PM, Dominik Vogt wrote:
> 
> >combine_simplify_rtx() tries to replace rtx expressions with just two
> >possible values with an experession that uses if_then_else:
> >
> >  (if_then_else (condition) (value1) (value2))
> >
> >If the original expression is e.g.
> >
> >  (and (reg) (const_int 2))
> 
> I'm not convinced that if_then_else_cond is the right place to do
> this. That function is designed to answer the question of whether an
> rtx has exactly one of two values and under which condition; I feel
> it should continue to work this way.
> 
> Maybe simplify_ternary_expression needs to be taught to deal with this case?

But simplify_ternary_expression isn't called with the following
test program (only tried it on s390x):

  void bar(int, int); 
  int foo(int a, int *b) 
  { 
    if (a) 
      bar(0, *b & 2); 
    return *b; 
  } 

combine_simplify_rtx() is called with 

  (sign_extend:DI (and:SI (reg:SI 61) (const_int 2)))

In the switch it calls simplify_unary_operation(), which return
NULL.  The next thing it does is call if_then_else_cond(), and
that calls itself with the sign_extend peeled off:

  (and:SI (reg:SI 61) (const_int 2))

takes the "BINARY_P (x)" path and returns false.  The problem
exists only if the (and ...) is wrapped in ..._extend, i.e. the
ondition dealing with (and ...) directly can be removed from the
patch.

So, all recursive calls to if_then_els_cond() return false, and
finally the condition in

    else if (HWI_COMPUTABLE_MODE_P (mode) 
           && pow2p_hwi (nz = nonzero_bits (x, mode))

is true.

Thus, if if_then_else_cond should remain unchanged, the only place
to fix this would be after the call to if_then_else_cond() in
combine_simplify_rtx().  Actually, there already is some special
case handling to override the return code of if_then_else_cond():

      cond = if_then_else_cond (x, &true_rtx, &false_rtx); 
      if (cond != 0 
          /* If everything is a comparison, what we have is highly unlikely 
             to be simpler, so don't use it.  */ 
--->      && ! (COMPARISON_P (x) 
                && (COMPARISON_P (true_rtx) || COMPARISON_P (false_rtx)))) 
        { 
          rtx cop1 = const0_rtx; 
          enum rtx_code cond_code = simplify_comparison (NE, &cond, &cop1); 
 
--->      if (cond_code == NE && COMPARISON_P (cond)) 
            return x; 
          ...

Should be easy to duplicate the test in the if-body, if that is
what you prefer:

          ...
          if (HWI_COMPUTABLE_MODE_P (GET_MODE (x)) 
              && pow2p_hwi (nz = nonzero_bits (x, GET_MODE (x))) 
              && ! ((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; 

(untested)

Ciao

Dominik ^_^  ^_^

-- 

Dominik Vogt
IBM Germany

Re: [PATCH] Do not simplify "(and (reg) (const bit))" to if_then_else.

[Dominik Vogt]

[-- Attachment #1: Type: text/plain, Size: 3313 bytes --]

On Fri, Nov 11, 2016 at 12:10:28PM +0100, Dominik Vogt wrote:
> On Mon, Nov 07, 2016 at 09:29:26PM +0100, Bernd Schmidt wrote:
> > On 10/31/2016 08:56 PM, Dominik Vogt wrote:
> > 
> > >combine_simplify_rtx() tries to replace rtx expressions with just two
> > >possible values with an experession that uses if_then_else:
> > >
> > >  (if_then_else (condition) (value1) (value2))
> > >
> > >If the original expression is e.g.
> > >
> > >  (and (reg) (const_int 2))
> > 
> > I'm not convinced that if_then_else_cond is the right place to do
> > this. That function is designed to answer the question of whether an
> > rtx has exactly one of two values and under which condition; I feel
> > it should continue to work this way.
> > 
> > Maybe simplify_ternary_expression needs to be taught to deal with this case?
> 
> But simplify_ternary_expression isn't called with the following
> test program (only tried it on s390x):
> 
>   void bar(int, int); 
>   int foo(int a, int *b) 
>   { 
>     if (a) 
>       bar(0, *b & 2); 
>     return *b; 
>   } 
> 
> combine_simplify_rtx() is called with 
> 
>   (sign_extend:DI (and:SI (reg:SI 61) (const_int 2)))
> 
> In the switch it calls simplify_unary_operation(), which return
> NULL.  The next thing it does is call if_then_else_cond(), and
> that calls itself with the sign_extend peeled off:
> 
>   (and:SI (reg:SI 61) (const_int 2))
> 
> takes the "BINARY_P (x)" path and returns false.  The problem
> exists only if the (and ...) is wrapped in ..._extend, i.e. the
> ondition dealing with (and ...) directly can be removed from the
> patch.
> 
> So, all recursive calls to if_then_els_cond() return false, and
> finally the condition in
> 
>     else if (HWI_COMPUTABLE_MODE_P (mode) 
>            && pow2p_hwi (nz = nonzero_bits (x, mode))
> 
> is true.
> 
> Thus, if if_then_else_cond should remain unchanged, the only place
> to fix this would be after the call to if_then_else_cond() in
> combine_simplify_rtx().  Actually, there already is some special
> case handling to override the return code of if_then_else_cond():
> 
>       cond = if_then_else_cond (x, &true_rtx, &false_rtx); 
>       if (cond != 0 
>           /* If everything is a comparison, what we have is highly unlikely 
>              to be simpler, so don't use it.  */ 
> --->      && ! (COMPARISON_P (x) 
>                 && (COMPARISON_P (true_rtx) || COMPARISON_P (false_rtx)))) 
>         { 
>           rtx cop1 = const0_rtx; 
>           enum rtx_code cond_code = simplify_comparison (NE, &cond, &cop1); 
>  
> --->      if (cond_code == NE && COMPARISON_P (cond)) 
>             return x; 
>           ...
> 
> Should be easy to duplicate the test in the if-body, if that is
> what you prefer:
> 
>           ...
>           if (HWI_COMPUTABLE_MODE_P (GET_MODE (x)) 
>               && pow2p_hwi (nz = nonzero_bits (x, GET_MODE (x))) 
>               && ! ((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; 
> 
> (untested)

Updated and tested version of the patch attached.  The extra logic
is now in combine_simplify_rtx.

Ciao

Dominik ^_^  ^_^

-- 

Dominik Vogt
IBM Germany

[-- Attachment #2: 0001-v2-ChangeLog --]
[-- Type: text/plain, Size: 129 bytes --]

gcc/ChangeLog

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

[-- Attachment #3: 0001-v2-Do-not-simplify-and-reg-const-bit-to-if_then_else.patch --]
[-- Type: text/plain, Size: 2036 bytes --]

From 2ebe692928b4ebee3fa6dc02136980801a04b33d Mon Sep 17 00:00:00 2001
From: Dominik Vogt <vogt@linux.vnet.ibm.com>
Date: Mon, 31 Oct 2016 09:00:31 +0100
Subject: [PATCH] Do not simplify "(and (reg) (const bit)" to if_then_else.

combine_simplify_rtx() tries to replace rtx expressions with just two
possible values with an experession that uses if_then_else:

  (if_then_else (condition) (value1) (value2))

If the original expression is e.g.

  (and (reg) (const_int 2))

where the constant is the mask for a single bit, the replacement results
in a more complex expression than before:

  (if_then_else (ne (zero_extract (reg) (1) (31))) (2) (0))

Similar replacements are done for

  (signextend (and ...))
  (zeroextend (and ...))

Suppress the replacement this special case in if_then_else_cond().
---
 gcc/combine.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

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;
+
 	  /* Simplify the alternative arms; this may collapse the true and
 	     false arms to store-flag values.  Be careful to use copy_rtx
 	     here since true_rtx or false_rtx might share RTL with x as a
-- 
2.3.0

Re: [PING] Do not simplify "(and (reg) (const bit))" to if_then_else.

[Dominik Vogt]

Ping.

On Mon, Nov 21, 2016 at 01:36:47PM +0100, Dominik Vogt wrote:
> On Fri, Nov 11, 2016 at 12:10:28PM +0100, Dominik Vogt wrote:
> > On Mon, Nov 07, 2016 at 09:29:26PM +0100, Bernd Schmidt wrote:
> > > On 10/31/2016 08:56 PM, Dominik Vogt wrote:
> > > 
> > > >combine_simplify_rtx() tries to replace rtx expressions with just two
> > > >possible values with an experession that uses if_then_else:
> > > >
> > > >  (if_then_else (condition) (value1) (value2))
> > > >
> > > >If the original expression is e.g.
> > > >
> > > >  (and (reg) (const_int 2))
> > > 
> > > I'm not convinced that if_then_else_cond is the right place to do
> > > this. That function is designed to answer the question of whether an
> > > rtx has exactly one of two values and under which condition; I feel
> > > it should continue to work this way.
> > > 
> > > Maybe simplify_ternary_expression needs to be taught to deal with this case?
> > 
> > But simplify_ternary_expression isn't called with the following
> > test program (only tried it on s390x):
> > 
> >   void bar(int, int); 
> >   int foo(int a, int *b) 
> >   { 
> >     if (a) 
> >       bar(0, *b & 2); 
> >     return *b; 
> >   } 
> > 
> > combine_simplify_rtx() is called with 
> > 
> >   (sign_extend:DI (and:SI (reg:SI 61) (const_int 2)))
> > 
> > In the switch it calls simplify_unary_operation(), which return
> > NULL.  The next thing it does is call if_then_else_cond(), and
> > that calls itself with the sign_extend peeled off:
> > 
> >   (and:SI (reg:SI 61) (const_int 2))
> > 
> > takes the "BINARY_P (x)" path and returns false.  The problem
> > exists only if the (and ...) is wrapped in ..._extend, i.e. the
> > ondition dealing with (and ...) directly can be removed from the
> > patch.
> > 
> > So, all recursive calls to if_then_els_cond() return false, and
> > finally the condition in
> > 
> >     else if (HWI_COMPUTABLE_MODE_P (mode) 
> >            && pow2p_hwi (nz = nonzero_bits (x, mode))
> > 
> > is true.
> > 
> > Thus, if if_then_else_cond should remain unchanged, the only place
> > to fix this would be after the call to if_then_else_cond() in
> > combine_simplify_rtx().  Actually, there already is some special
> > case handling to override the return code of if_then_else_cond():
> > 
> >       cond = if_then_else_cond (x, &true_rtx, &false_rtx); 
> >       if (cond != 0 
> >           /* If everything is a comparison, what we have is highly unlikely 
> >              to be simpler, so don't use it.  */ 
> > --->      && ! (COMPARISON_P (x) 
> >                 && (COMPARISON_P (true_rtx) || COMPARISON_P (false_rtx)))) 
> >         { 
> >           rtx cop1 = const0_rtx; 
> >           enum rtx_code cond_code = simplify_comparison (NE, &cond, &cop1); 
> >  
> > --->      if (cond_code == NE && COMPARISON_P (cond)) 
> >             return x; 
> >           ...
> > 
> > Should be easy to duplicate the test in the if-body, if that is
> > what you prefer:
> > 
> >           ...
> >           if (HWI_COMPUTABLE_MODE_P (GET_MODE (x)) 
> >               && pow2p_hwi (nz = nonzero_bits (x, GET_MODE (x))) 
> >               && ! ((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; 
> > 
> > (untested)
> 
> Updated and tested version of the patch attached.  The extra logic
> is now in combine_simplify_rtx.

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

> >From 2ebe692928b4ebee3fa6dc02136980801a04b33d Mon Sep 17 00:00:00 2001
> From: Dominik Vogt <vogt@linux.vnet.ibm.com>
> Date: Mon, 31 Oct 2016 09:00:31 +0100
> Subject: [PATCH] Do not simplify "(and (reg) (const bit)" to if_then_else.
> 
> combine_simplify_rtx() tries to replace rtx expressions with just two
> possible values with an experession that uses if_then_else:
> 
>   (if_then_else (condition) (value1) (value2))
> 
> If the original expression is e.g.
> 
>   (and (reg) (const_int 2))
> 
> where the constant is the mask for a single bit, the replacement results
> in a more complex expression than before:
> 
>   (if_then_else (ne (zero_extract (reg) (1) (31))) (2) (0))
> 
> Similar replacements are done for
> 
>   (signextend (and ...))
>   (zeroextend (and ...))
> 
> Suppress the replacement this special case in if_then_else_cond().
> ---
>  gcc/combine.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> 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;
> +
>  	  /* Simplify the alternative arms; this may collapse the true and
>  	     false arms to store-flag values.  Be careful to use copy_rtx
>  	     here since true_rtx or false_rtx might share RTL with x as a
> -- 
> 2.3.0
> 



Ciao

Dominik ^_^  ^_^

-- 

Dominik Vogt
IBM Germany

Re: [PATCH] Do not simplify "(and (reg) (const bit))" to if_then_else.

[Bernd Schmidt]

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.


Bernd

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

[Dominik Vogt]

[-- Attachment #1: Type: text/plain, Size: 1459 bytes --]

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 ^_^  ^_^

-- 

Dominik Vogt
IBM Germany

[-- Attachment #2: 0001-v3-ChangeLog --]
[-- Type: text/plain, Size: 129 bytes --]

gcc/ChangeLog

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

[-- Attachment #3: 0001-v3-Do-not-simplify-and-reg-const-bit-to-if_then_else.patch --]
[-- Type: text/plain, Size: 1815 bytes --]

From 9202cab6332ce5dcfa740bbae3bcf07f3acc8705 Mon Sep 17 00:00:00 2001
From: Dominik Vogt <vogt@linux.vnet.ibm.com>
Date: Mon, 31 Oct 2016 09:00:31 +0100
Subject: [PATCH] Do not simplify "(and (reg) (const bit)" to if_then_else.

combine_simplify_rtx() tries to replace rtx expressions with just two
possible values with an experession that uses if_then_else:

  (if_then_else (condition) (value1) (value2))

If the original expression is e.g.

  (and (reg) (const_int 2))

where the constant is the mask for a single bit, the replacement results
in a more complex expression than before:

  (if_then_else (ne (zero_extract (reg) (1) (31))) (2) (0))

Similar replacements are done for

  (signextend (and ...))
  (zeroextend (and ...))

Suppress the replacement this special case in if_then_else_cond().
---
 gcc/combine.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

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
-- 
2.3.0

Re: [PATCH] Do not simplify "(and (reg) (const bit))" to if_then_else.

[Jeff Law]

On 11/21/2016 05:36 AM, Dominik Vogt wrote:
> On Fri, Nov 11, 2016 at 12:10:28PM +0100, Dominik Vogt wrote:
>> > On Mon, Nov 07, 2016 at 09:29:26PM +0100, Bernd Schmidt wrote:
>>> > > On 10/31/2016 08:56 PM, Dominik Vogt wrote:
>>> > >
>>>> > > >combine_simplify_rtx() tries to replace rtx expressions with just two
>>>> > > >possible values with an experession that uses if_then_else:
>>>> > > >
>>>> > > >  (if_then_else (condition) (value1) (value2))
>>>> > > >
>>>> > > >If the original expression is e.g.
>>>> > > >
>>>> > > >  (and (reg) (const_int 2))
>>> > >
>>> > > I'm not convinced that if_then_else_cond is the right place to do
>>> > > this. That function is designed to answer the question of whether an
>>> > > rtx has exactly one of two values and under which condition; I feel
>>> > > it should continue to work this way.
>>> > >
>>> > > Maybe simplify_ternary_expression needs to be taught to deal with this case?
>> >
>> > But simplify_ternary_expression isn't called with the following
>> > test program (only tried it on s390x):
>> >
>> >   void bar(int, int);
>> >   int foo(int a, int *b)
>> >   {
>> >     if (a)
>> >       bar(0, *b & 2);
>> >     return *b;
>> >   }
>> >
>> > combine_simplify_rtx() is called with
>> >
>> >   (sign_extend:DI (and:SI (reg:SI 61) (const_int 2)))
>> >
>> > In the switch it calls simplify_unary_operation(), which return
>> > NULL.  The next thing it does is call if_then_else_cond(), and
>> > that calls itself with the sign_extend peeled off:
>> >
>> >   (and:SI (reg:SI 61) (const_int 2))
>> >
>> > takes the "BINARY_P (x)" path and returns false.  The problem
>> > exists only if the (and ...) is wrapped in ..._extend, i.e. the
>> > ondition dealing with (and ...) directly can be removed from the
>> > patch.
>> >
>> > So, all recursive calls to if_then_els_cond() return false, and
>> > finally the condition in
>> >
>> >     else if (HWI_COMPUTABLE_MODE_P (mode)
>> >            && pow2p_hwi (nz = nonzero_bits (x, mode))
>> >
>> > is true.
>> >
>> > Thus, if if_then_else_cond should remain unchanged, the only place
>> > to fix this would be after the call to if_then_else_cond() in
>> > combine_simplify_rtx().  Actually, there already is some special
>> > case handling to override the return code of if_then_else_cond():
>> >
>> >       cond = if_then_else_cond (x, &true_rtx, &false_rtx);
>> >       if (cond != 0
>> >           /* If everything is a comparison, what we have is highly unlikely
>> >              to be simpler, so don't use it.  */
>> > --->      && ! (COMPARISON_P (x)
>> >                 && (COMPARISON_P (true_rtx) || COMPARISON_P (false_rtx))))
>> >         {
>> >           rtx cop1 = const0_rtx;
>> >           enum rtx_code cond_code = simplify_comparison (NE, &cond, &cop1);
>> >
>> > --->      if (cond_code == NE && COMPARISON_P (cond))
>> >             return x;
>> >           ...
>> >
>> > Should be easy to duplicate the test in the if-body, if that is
>> > what you prefer:
>> >
>> >           ...
>> >           if (HWI_COMPUTABLE_MODE_P (GET_MODE (x))
>> >               && pow2p_hwi (nz = nonzero_bits (x, GET_MODE (x)))
>> >               && ! ((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;
>> >
>> > (untested)
> Updated and tested version of the patch attached.  The extra logic
> is now in combine_simplify_rtx.
>
> Ciao
>
> Dominik ^_^  ^_^
>
> -- Dominik Vogt IBM Germany
>
>
> 0001-v2-ChangeLog
>
>
> gcc/ChangeLog
>
> 	* combine.c (combine_simplify_rtx):  Suppress replacement of
> 	"(and (reg) (const_int bit))" with "if_then_else".
I'd agree with Bernd that if_then_else_cond would be the wrong place to 
do this.

The PA could implement something like:


(if_then_else (ne (zero_extract (reg) (1) (31))) (X) (0))

For any many constants very efficiently.  But it's a dead architecture 
and we won't have any define_insns in place to do that :-)

Anyway, the patch is OK for the trunk.  It's hard to see how a simple 
(and (X) (C)) -> (if_then_else (condition) (val1) (val2)) is a good 
transformation to make :-)

Jeff

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

[Andreas Krebbel]

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-

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

[Andreas Schwab]

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.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

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

[Segher Boessenkool]

[ 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

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

[Dominik Vogt]

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 ^_^  ^_^

-- 

Dominik Vogt
IBM Germany

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

[Segher Boessenkool]

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

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

[Dominik Vogt]

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 ^_^  ^_^

-- 

Dominik Vogt
IBM Germany

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

[Oleg Endo]

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

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

[Segher Boessenkool]

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

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

[Dominik Vogt]

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 ^_^  ^_^

-- 

Dominik Vogt
IBM Germany