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

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

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

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

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