fold bug with multiply and cast and conditional

fold bug with multiply and cast and conditional

[Andrew Pinski]

I noticed this will working on my tree combiner, I don't know if I can 
reproduce it
with any front-end though.

Take the following code:
int f(int a, int *b)
{
   _Bool a1 = (a!=0);
   return a1 * 4;
}

When I do my combining I produce a tree like:
<mult_expr 0x4240f384
     type integer_type
     arg 0 <convert_expr 0x42409f20 type <integer_type 0x4241164c int>
         arg 0 <ne_expr 0x4240f360 type <boolean_type 0x42411934 _Bool>
             arg 0 <ssa_name 0x4240f45c type integer_type>
             arg 1 <integer_cst 0x42409a20 constant invariant 0>>>
     arg 1 <integer_cst 0x42409d20 type integer_type >

but we fold that to 0:
<integer_cst 0x42409a20 type <integer_type 0x4241164c int> constant 
invariant 0>

This seems wrong.

Note this bug causes a large number of regressions with my tree 
combiner enabled.

Also looking at the code it looks like we never set the variables lhs 
and rhs
for the non COND_EXPR case.

Also I think this is a regression from 3.4.x where we do the correct 
thing for
this case.
I think this was changed with the following patch:
2004-05-20  Roger Sayle  <roger@eyesopen.com>

         PR middle-end/3074
         * fold-const.c (strip_compound_expr): Delete function.
         (count_cond): Delete function.
         (fold_binary_op_with_conditional_arg): Only perform 
transformations
         "a + (b?c:d) -> b ? a+c : a+d" and "(b?c:d) + a -> b ? c+a : 
d+a"
         when a is constant.  This greatly simplifies this routine.

         * tree.c (saved_expr_p): Delete function.
         * tree.h (saved_expr_p): Delete function prototype.


Thanks,
Andrew Pinski

Re: fold bug with multiply and cast and conditional

[Roger Sayle]

Hi Andrew,

On Wed, 8 Dec 2004, Andrew Pinski wrote:
> Also looking at the code it looks like we never set the variables lhs
> and rhs for the non COND_EXPR case.  Also I think this is a regression
> from 3.4.x where we do the correct thing for this case.
> I think this was changed with the following patch:
> 2004-05-20  Roger Sayle  <roger@eyesopen.com>
>
>       PR middle-end/3074
>       * fold-const.c (strip_compound_expr): Delete function.
>       (fold_binary_op_with_conditional_arg): Only perform transformations
>       "a + (b?c:d) -> b ? a+c : a+d" and "(b?c:d) + a -> b ? c+a : d+a"
>       when a is constant.  This greatly simplifies this routine.

I think you're misattributing your the problem to the function
fold_binary_op_with_conditional_arg, though clearly you haven't
given enough information to determine where things are actually
going  wrong.

In fold_binary_op_with_conditional_arg, the variables "lhs" and "rhs"
are initially set to NULL_TREE at the top of the function.  The lhs
and rhs are only assigned values in the COND_EXPR case, for exceptional
circumstances where "lhs" and/or "rhs" are assigned a tree of void
type to override the usual case processing below.

The normal case processing then happens after the COND_EXPR/boolean
cases where we check whether "lhs" and "rhs" are still NULL (i.e. they
don't throw an exception or fail to return), in which case we construct
them as we should.

Hopefully, the above explanation should be clear enough, and show
that the current implementation of fold_binary_op_with_condition_arg
is correct.  Fortunately, my patch you quote above, made this function
much earier to read than it was previously.  But even as written it
should be clear that lhs and rhs are never used without previously
being assigned an appropriate value.


Best of luck debugging your problem, wherever it may be :>  My initial
guess would be that your combiner is not correctly casting the types of
the operands of the multiplication to the appropriate/result type.
Just a guess, but this would explain why you can't reproduce it with
any of GCC's language front-ends, which correctly preserve tree type
safety.

Roger
--

Re: fold bug with multiply and cast and conditional

[Andrew Pinski]

On Dec 8, 2004, at 11:03 PM, Roger Sayle wrote:

> Best of luck debugging your problem, wherever it may be :>  My initial
> guess would be that your combiner is not correctly casting the types of
> the operands of the multiplication to the appropriate/result type.
> Just a guess, but this would explain why you can't reproduce it with
> any of GCC's language front-ends, which correctly preserve tree type
> safety.

Sorry about misattributing this problem to your patch.  I should looked
into the problem a little further.

It was due to the wrong type but not by my tree combiner.  The
bug is in fold_binary_op_with_conditional_arg still.  We take
the type of the conditional to make true/false_value instead of
using the type of the outer operation so we get a miss-matched type.

It also explains why I cannot reproduce it with the front-ends (well I
only tried the C based ones) but the front-ends do not produce a
straight up conversion to int from a condition, but instead they
(really convert) creates a COND_EXPR which means we don't hit this
part of the code also.

This is the patch which I am testing which should fix the bug, this is a
regression in a sense from 2.95.3 which did not have
fold_binary_op_with_conditional_arg.

Index: fold-const.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/fold-const.c,v
retrieving revision 1.482
diff -u -p -r1.482 fold-const.c
--- fold-const.c	30 Nov 2004 20:33:34 -0000	1.482
+++ fold-const.c	9 Dec 2004 05:29:08 -0000
@@ -5392,10 +5392,9 @@ fold_binary_op_with_conditional_arg (enu
      }
    else
      {
-      tree testtype = TREE_TYPE (cond);
        test = cond;
-      true_value = constant_boolean_node (true, testtype);
-      false_value = constant_boolean_node (false, testtype);
+      true_value = constant_boolean_node (true, type);
+      false_value = constant_boolean_node (false, type);
      }

    if (lhs == 0)


Thanks,
Andrew Pinski

Re: fold bug with multiply and cast and conditional

[Roger Sayle]

On Thu, 9 Dec 2004, Andrew Pinski wrote:
> @@ -5392,10 +5392,9 @@ fold_binary_op_with_conditional_arg (enu
>       }
>     else
>       {
> -      tree testtype = TREE_TYPE (cond);
>         test = cond;
> -      true_value = constant_boolean_node (true, testtype);
> -      false_value = constant_boolean_node (false, testtype);
> +      true_value = constant_boolean_node (true, type);
> +      false_value = constant_boolean_node (false, type);
>       }
>

Hmm.  I guess this is Ok if we can't trust that "cond" has type
"type".  If you're going to make this change, might I also then
recommend the symmetrical change to not assume the other operand
"arg" is of type "type" either.  i.e. add the line

	arg = fold_convert (type, arg);

either just after the "if (!TREE_CONSTANT (arg))" test, or just
before the "if (lhs == 0)" test.  We know "arg" is a constant,
so this just ensures it has the right type, and shouldn't obfuscate
the trees we generate.


Post the full patch to gcc-patches with a ChangeLog after you've
bootstrapped and regression tested it, and I'll take a look.

Roger
--