[patch] Fix PHI optimization issue with boolean types

[Jeff Law]

On 10/18/2016 02:35 AM, Richard Biener wrote:
> 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.
>>
>> 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?
>
> Hmm, I wonder if the patch is a good approach as you are likely only papering
> over a single of possibly very many affected places (wi::fits_to_tree_p comes
> immediately to my mind).  I suppose a better way would be for Ada to not
> lie about those types and not use BOOLEAN_TYPE but INTEGER_TYPE.
> Because BOOLEAN_TYPE types only have two values as documented in
> tree.def:
>
> /* Boolean type (true or false are the only values).  Looks like an
>    INTEGRAL_TYPE.  */
> DEFTREECODE (BOOLEAN_TYPE, "boolean_type", tcc_type, 0)
>
> There are not many references to BOOLEAN_TYPE in ada/gcc-interface
> thus it shouldn't be hard to change ... (looks like Ada might even prefer
> ENUMERAL_TYPE here).
>
> Thanks,
> Richard.
>
>>
>> 2016-10-18  Eric Botcazou  <ebotcazou@adacore.com>
>>
>>         * tree.c (int_fits_type_p): Accept only 0 and 1 for boolean types.
Alternately we could check the precision/range in the optimizers.  We do 
that in various places because of this exact issue, I could well have 
missed one or more.

Though I would have a preference for fixing int_fits_type_p which seems 
like it'll catch the cases we care about without having to twiddle every 
optimizer.

jeff