[patch] Fix PHI optimization issue with boolean types

[Richard Biener]

On Tue, Oct 18, 2016 at 8:35 AM, Eric Botcazou <ebotcazou@adacore.com> wrote:
> Hi,
>
> this is a regression present on the mainline and 6 branch: the compiler now
> generates wrong code for the attached testcase at -O because of an internal
> conflict about boolean types.  The sequence is as follows.  In .mergephi3:
>
>   boolean _22;
>   p__enum res;
>
>   <bb 9>:
>   if (_22 != 0)
>     goto <bb 10>;
>   else
>     goto <bb 11>;
>
>   <bb 10>:
>
>   <bb 11>:
>   # res_17 = PHI <2(8), 0(9), 1(10)>
>
> is turned into:
>
> COND_EXPR in block 9 and PHI in block 11 converted to straightline code.
> PHI res_17 changed to factor conversion out from COND_EXPR.
> New stmt with CAST that defines res_17.
>
>   boolean _33;
>
>   <bb 10>:
>   # _33 = PHI <2(8), _22(9)>
>   res_17 = (p__enum) _33;
>
> [...]
>
>   <bb 12>:
>   if (res_17 != 0)
>     goto <bb 13>;
>   else
>     goto <bb 14>;
>
>   <bb 13>:
>   _12 = res_17 == 2;
>   _13 = (integer) _12
>
> in .phiopt1.  So boolean _33 can have value 2.  Later forwprop3 propagates _33
> into the uses of res_17:
>
>   <bb 12>:
>   if (_33 != 0)
>     goto <bb 13>;
>   else
>     goto <bb 14>;
>
>   <bb 13>:
>   _12 = _33 == 2;
>   _13 = (integer) _12;
>
> and DOM3 deduces:
>
>   <bb 13>:
>   _12 = 0;
>   _13 = 0;
>
> because it computes that _33 has value 1 in BB 13 since it's a boolean.

Btw, isn't this the issue?  You'd need to avoid this as well I guess:

          /* Special case comparing booleans against a constant as we
             know the value of OP0 on both arms of the branch.  i.e., we
             can record an equivalence for OP0 rather than COND.

             However, don't do this if the constant isn't zero or one.
             Such conditionals will get optimized more thoroughly during
             the domwalk.  */
          if ((code == EQ_EXPR || code == NE_EXPR)
              && TREE_CODE (op0) == SSA_NAME
              && ssa_name_has_boolean_range (op0)
              && is_gimple_min_invariant (op1)
              && (integer_zerop (op1) || integer_onep (op1)))
            {
...

and thus:

bool
ssa_name_has_boolean_range (tree op)
{
  gcc_assert (TREE_CODE (op) == SSA_NAME);

  /* Boolean types always have a range [0..1].  */
  if (TREE_CODE (TREE_TYPE (op)) == BOOLEAN_TYPE)
    return true;

as said, there are _very_ many places in the compiler you'd need to "fix".

Basically treating BOOLEAN_TYPE this way is to cover frontends like
fortran who have multiple boolean types of different size (and not precision 1
IIRC).  An alternative would be to track them all down and force their
precision to 1 with unknown fallout.  And then document BOOLEAN_TYPE
isn't really special anymore (and fix all places).  But then, why do we have
BOOLEAN_TYPE ...

Richard.

> The problem was introduced by the new factor_out_conditional_conversion:
>
>       /* If arg1 is an INTEGER_CST, fold it to new type.  */
>       if (INTEGRAL_TYPE_P (TREE_TYPE (new_arg0))
>           && int_fits_type_p (arg1, TREE_TYPE (new_arg0)))
>         {
>           if (gimple_assign_cast_p (arg0_def_stmt))
>             new_arg1 = fold_convert (TREE_TYPE (new_arg0), arg1);
>           else
>             return NULL;
>         }
>       else
>         return NULL
>
> int_fits_type_p is documented as taking only INTEGER_TYPE, but is invoked
> on constant 2 and a BOOLEAN_TYPE and returns true.
>
> BOOLEAN_TYPE is special in Ada: it has precision 8 and range [0;255] so the
> outcome of int_fits_type_p is not unreasonable.  But this goes against the
> various transformations applied to boolean types in the compiler, which all
> assume that they can only take values 0 or 1.
>
> Hence the attached fix (which should be a no-op except for Ada), tested on
> x86_64-suse-linux, OK for mainline and 6 branch?
>
>
> 2016-10-18  Eric Botcazou  <ebotcazou@adacore.com>
>
>         * tree.c (int_fits_type_p): Accept only 0 and 1 for boolean types.
>
>
> 2016-10-18  Eric Botcazou  <ebotcazou@adacore.com>
>
>         * gnat.dg/opt59.adb: New test.
>         * gnat.dg/opt59_pkg.ad[sb]: New helper.
>
> --
> Eric Botcazou