| Message ID | 10860828.opkyFOrRK6@polaris |
|---|---|
| State | Superseded |
| Headers | show |
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. > > > 2016-10-18 Eric Botcazou <ebotcazou@adacore.com> > > * gnat.dg/opt59.adb: New test. > * gnat.dg/opt59_pkg.ad[sb]: New helper. > > -- > Eric Botcazou
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
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 >
> 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) Yes, but on the other hand they have a precision and a range. > 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). It was like that originally (an ENUMERAL_TYPE) but this blocked LTO (well, cross-language LTO) exactly like the sizetype discrepancy, so I don't really feel like going back in time. -- Eric Botcazou
On Thu, Oct 20, 2016 at 12:50 AM, Eric Botcazou <ebotcazou@adacore.com> wrote: >> 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) > > Yes, but on the other hand they have a precision and a range. > >> 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). > > It was like that originally (an ENUMERAL_TYPE) but this blocked LTO (well, > cross-language LTO) exactly like the sizetype discrepancy, so I don't really > feel like going back in time. Because boolean_type_node was different? Well, are really _all_ boolean types this way or can you keep using BOOLEAN_TYPE for boolean_type_node for things like interfacing to C and C++ (and the builtins, etc.) and use ENUMERAL_TYPE for the cases where bool can have non zero/one values? Richard. > -- > Eric Botcazou
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. 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". 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 ... Richard. > 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? > > > 2016-10-18 Eric Botcazou <ebotcazou@adacore.com> > > * tree.c (int_fits_type_p): Accept only 0 and 1 for boolean types. > > > 2016-10-18 Eric Botcazou <ebotcazou@adacore.com> > > * gnat.dg/opt59.adb: New test. > * gnat.dg/opt59_pkg.ad[sb]: New helper. > > -- > Eric Botcazou
> Because boolean_type_node was different? Well, are really _all_ boolean > types this way or can you keep using BOOLEAN_TYPE for boolean_type_node for > things like interfacing to C and C++ (and the builtins, etc.) and use > ENUMERAL_TYPE for the cases where bool can have non zero/one values? They are all like that. In a valid program, they can have only zero or one as values but, unlike C, in Ada you cannot avert your yes on undefined behaviour, you need to handle it gracefully enough, hence the need to have a base type for them (i.e. size of mode == precision). ENUMERAL_TYPE would not really work either for the same reason. So we would need INTEGER_TYPE for strict correctness, but this would add casts from/to boolean_type_node all over the place so is not very appealing either. -- Eric Botcazou
> 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. > 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
On Fri, Oct 21, 2016 at 9:40 AM, Eric Botcazou <ebotcazou@adacore.com> 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
> 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? Yes, that would certainly solve the problem too. No strong opinion, my proposed fix was a compromise aimed at changing nothing except for Ada. > 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? No, BOOLEAN_TYPEs are base types in Ada so must have maximal range. > For your patch, can you use integer_zerop () || integer_truep () and also > fix wi::fits_to_tree_p? But integer_truep is just integer_onep for BOOLEAN_TYPEs. And fits_to_tree_p doesn't really need to be changed if int_fits_type_p is and the API layering is properly enforced, does it? > 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? In a valid program, no. In an invalid program, maybe, in which case we don't want to ICE so we would need to filter them out and raise Constraint_Error instead when we run into one; certainly doable I'd say. -- Eric Botcazou
On Mon, Oct 24, 2016 at 11:15 AM, Eric Botcazou <ebotcazou@adacore.com> wrote: >> 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? > > Yes, that would certainly solve the problem too. No strong opinion, my > proposed fix was a compromise aimed at changing nothing except for Ada. > >> 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? > > No, BOOLEAN_TYPEs are base types in Ada so must have maximal range. > >> For your patch, can you use integer_zerop () || integer_truep () and also >> fix wi::fits_to_tree_p? > > But integer_truep is just integer_onep for BOOLEAN_TYPEs. Yes, but it's more descriptive IMHO. > And fits_to_tree_p > doesn't really need to be changed if int_fits_type_p is and the API layering > is properly enforced, does it? Well. fits_to_tree_p avoids creating an INTEGER_CST in ggc memory and thus is the prefered way to test if you have a wide-int but not yet an INTEGER_CST. >> 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? > > In a valid program, no. In an invalid program, maybe, in which case we don't > want to ICE so we would need to filter them out and raise Constraint_Error > instead when we run into one; certainly doable I'd say. Ok, just looking for a way to "easily" fend off other cases that are not catched by patching the fits-tree routines. Richard. > -- > Eric Botcazou
Index: tree.c =================================================================== --- tree.c (revision 241294) +++ tree.c (working copy) @@ -9065,8 +9065,8 @@ get_narrower (tree op, int *unsignedp_pt return win; } -/* Returns true if integer constant C has a value that is permissible - for type TYPE (an INTEGER_TYPE). */ +/* Return true if integer constant C has a value that is permissible + for TYPE, an integral type. */ bool int_fits_type_p (const_tree c, const_tree type) @@ -9075,6 +9075,11 @@ int_fits_type_p (const_tree c, const_tre bool ok_for_low_bound, ok_for_high_bound; signop sgn_c = TYPE_SIGN (TREE_TYPE (c)); + /* Short-circuit boolean types since various transformations assume that + they can only take values 0 and 1. */ + if (TREE_CODE (type) == BOOLEAN_TYPE) + return integer_zerop (c) || integer_onep (c); + retry: type_low_bound = TYPE_MIN_VALUE (type); type_high_bound = TYPE_MAX_VALUE (type);
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? 2016-10-18 Eric Botcazou <ebotcazou@adacore.com> * tree.c (int_fits_type_p): Accept only 0 and 1 for boolean types. 2016-10-18 Eric Botcazou <ebotcazou@adacore.com> * gnat.dg/opt59.adb: New test. * gnat.dg/opt59_pkg.ad[sb]: New helper. -- Eric Botcazou