On Tue, Oct 18, 2016 at 4:10 PM, Jeff Law <l...@redhat.com> wrote:
> On 10/18/2016 02:35 AM, Richard Biener wrote:
>>
>> On Tue, Oct 18, 2016 at 8:35 AM, Eric Botcazou <ebotca...@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 <ebotca...@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
>
