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 >