On Fri, Oct 21, 2016 at 9:40 AM, Eric Botcazou <[email protected]> wrote:
>> 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".
>
> I don't think so, you just need to prevent the compiler from creating boolean
> values that are not zero and one, there is none in a valid Ada program. This
> has worked perfectly for years, it's because factor_out_conditional_conversion
> breaks the documented interface of int_fits_type_p by sending it a boolean
> type instead of an integer type that the regression was introduced. This was
> unfortunatelty not caught in the review, so we need to do damage control now,
> including on the 6 branch, and my patch is a rather safe way of doing it.
Sure it's "safe", but I pointed you to wi::fits_tree_p which is the more modern
variant of int_fits_type_p - you at least should fix that for
consistency as well.
Btw, if int_fits_type_p is supposed to be only called for INTEGER_TYPEs
then
/* 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;
}
should maybe be changed to check for INTEGER_TYPE instead of
INTEGRAL_TYPE_P? And int_fits_type_p should have an assert
asserting it is fed only INTEGER_TYPEs?
Alternatively int_fits_type_p looks at TYPE_MIN/MAX_VALUE, would
it work to set that to [0, 1] for your Ada BOOLEAN_TYPEs?
For your patch, can you use integer_zerop () || integer_truep () and also
fix wi::fits_to_tree_p?
Would it work to have wide_int_to_tree check that we never generate
a BOOLEAN_TYPE constant that is not zero or one? That is, does
the Ada FE create those?
Thanks,
Richard.
>> 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 ...
>
> I think BOOLEAN_TYPE essentially works fine for Ada if you don't try to treat
> it like an INTEGER_TYPE, as the new factor_out_conditional_conversion does...
> --
> Eric Botcazou
