diff mbox

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

Message ID 20161121123647.GA22233@linux.vnet.ibm.com
State New
Headers show

Commit Message

Dominik Vogt Nov. 21, 2016, 12:36 p.m. UTC 
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 ^_^  ^_^

Comments

Dominik Vogt Dec. 1, 2016, 12:19 p.m. UTC | #1 
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 ^_^  ^_^
Bernd Schmidt Dec. 1, 2016, 12:33 p.m. UTC | #2 
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
Jeff Law Dec. 1, 2016, 11:49 p.m. UTC | #3 
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
diff mbox

Patch

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