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

Jeff Law law@redhat.com
Thu Dec 1 23:49:00 GMT 2016


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



More information about the Gcc-patches mailing list