[patch] Fix PHI optimization issue with boolean types

Richard Biener richard.guenther@gmail.com
Wed Oct 19 10:05:00 GMT 2016


On Tue, Oct 18, 2016 at 4:10 PM, Jeff Law <law@redhat.com> wrote:
> 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.

I don't think we do.  Instead we have various places that do

  if ((TREE_CODE (TREE_TYPE (rhs1)) == BOOLEAN_TYPE
       || (INTEGRAL_TYPE_P (TREE_TYPE (rhs1))
           && TYPE_PRECISION (TREE_TYPE (rhs1)) == 1))

which basically means BOOLEAN_TYPEs have two values only regardless of
their precision or mode.

> 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.

It doesn't catch the above.  If BOOLEAN_TYPE is not special why do we have
it at all?

Richard.

> jeff
>



More information about the Gcc-patches mailing list