| Message ID | 201105141814.10822.ebotcazou@adacore.com |
|---|---|
| State | New |
| Headers | show |
2011/5/14 Eric Botcazou <ebotcazou@adacore.com>: >> Those issues should be fixed by the attached patch, which relaxes >> strictness of logical operations in tree-cfg.c file. > > Thanks. > >> 2011-05-14 Kai Tietz >> >> * tree-cfg.c (verify_gimple_assign_unary): Don't enforce >> boolean_type_node >> compatible lhs/rhs types for logical expression. >> (verify_gimple_assign_binary): Likewise. > > - /* We allow only boolean typed or compatible argument and result. */ > - if (!useless_type_conversion_p (boolean_type_node, rhs1_type) > - || !useless_type_conversion_p (boolean_type_node, rhs2_type) > - || !useless_type_conversion_p (boolean_type_node, lhs_type)) > + /* The gimplify code ensures that just boolean_type_node types > + are present, but ADA and FORTRAN using artificial gimple generation > + code which is producing non-gimplify compatible trees. So we need > + to allow here any kind of integral typed argument and result. */ > + if (!INTEGRAL_TYPE_P (rhs1_type) > + || !INTEGRAL_TYPE_P (rhs2_type) > + || !INTEGRAL_TYPE_P (lhs_type)) > > What does "artificial gimple generation code" mean exactly? The only thing > different in Ada is the definition of boolean_type_node which isn't compatible > with the C definition (its precision is 8 instead of 1). That's all. Well, I mean by artificial here, that gimplification is done via gimplify_expr API. As FE and ME have here different assumptions. The ME uses internally most boolean_type_node and IMHO it should be the variant used there. As this conversation to a single boolean_type (with recast to result FE's boolean type on demand) has some advantages on optimization passes. Additionally it simplifies logic in passes on types. For example there are some expressions, which are in general unexpected in ME as they are transformed in gimplification (like TRUTH_ANDIF/ORIF_EXPR). By adding tree manual, you might cause the same issue as for the logical-expression showing up now. > For Ada, you can just do: > > Index: tree-cfg.c > =================================================================== > --- tree-cfg.c (revision 173756) > +++ tree-cfg.c (working copy) > @@ -3350,7 +3350,7 @@ verify_gimple_assign_unary (gimple stmt) > return false; > > case TRUTH_NOT_EXPR: > - if (!useless_type_conversion_p (boolean_type_node, rhs1_type)) > + if (TREE_CODE (rhs1_type) != BOOLEAN_TYPE) > { > error ("invalid types in truth not"); > debug_generic_expr (lhs_type); > @@ -3558,10 +3558,10 @@ do_pointer_plus_expr_check: > case TRUTH_OR_EXPR: > case TRUTH_XOR_EXPR: > { > - /* We allow only boolean typed or compatible argument and result. */ > - if (!useless_type_conversion_p (boolean_type_node, rhs1_type) > - || !useless_type_conversion_p (boolean_type_node, rhs2_type) > - || !useless_type_conversion_p (boolean_type_node, lhs_type)) > + /* We allow only boolean-typed argument and result. */ > + if (TREE_CODE (rhs1_type) != BOOLEAN_TYPE > + || TREE_CODE (rhs2_type) != BOOLEAN_TYPE > + || TREE_CODE (lhs_type) != BOOLEAN_TYPE) > { > error ("type mismatch in binary truth expression"); > debug_generic_expr (lhs_type); > > -- > Eric Botcazou > Well, this patch might be an alternative, but I see here potential issues in such none-gimplified expressions for comparision and logical not, which not necessariily have BOOLEAN_TYPE. See here the code for fold_truth_not (and some other places) in fold-const. So I think, as long as we have here external gimplication it is more save to check just for integral-kind. At the moment I am discussing with Tobias Burnus about wrapping fold_build calls for logical operations (COND_EXPR (conditional operand); TRUTH..., and possibly comparison too) , so that the operation is done on boolean_type_node and the result gets re-casted back to FE's logical boolean type. This would allow us to keep in tree-cfg the check for one-bit operations for truth-expressions. The general advantage here is that later passes (like reassoc - by moving TRUTH-AND/OR/XOR to binary variant BIT_AND/OR/XOR) would show effects and inner operands can be matched. Also by oiperating on one-bit types here allows us to assume even that TRUTH_NOT_EXPR can be transformed to BIT_NOT_EXPR (or XOR by 1). But well, I would like to know Richard's opinion here. Regards, Kai
> Well, I mean by artificial here, that gimplification is done via > gimplify_expr API. As FE and ME have here different assumptions. The > ME uses internally most boolean_type_node and IMHO it should be the > variant used there. As this conversation to a single boolean_type > (with recast to result FE's boolean type on demand) has some > advantages on optimization passes. Additionally it simplifies logic > in passes on types. For example there are some expressions, which are > in general unexpected in ME as they are transformed in gimplification > (like TRUTH_ANDIF/ORIF_EXPR). By adding tree manual, you might cause > the same issue as for the logical-expression showing up now. OK, then that's definitely not the case for Ada, so the comment is incorrect. > Well, this patch might be an alternative, but I see here potential > issues in such none-gimplified expressions for comparision and logical > not, which not necessariily have BOOLEAN_TYPE. See here the code for > fold_truth_not (and some other places) in fold-const. So I think, as > long as we have here external gimplication it is more save to check > just for integral-kind. Note that, even without "external gimplication", we still have integral types down to the tree-cfg.c check. Take ACATS c52103x at -O0. The Ada FE hands over a valid TRUTH_AND_EXPR, i.e. (BOOLEAN_TYPE, BOOLEAN_TYPE, BOOLEAN_TYPE) but the gimplifier builds a (BOOLEAN_TYPE, INTEGER_TYPE, BOOLEAN_TYPE) as it strips, then adds, then re-strips a cast to BOOLEAN_TYPE in gimplify_expr.
2011/5/15 Eric Botcazou <ebotcazou@adacore.com>: >> Well, I mean by artificial here, that gimplification is done via >> gimplify_expr API. As FE and ME have here different assumptions. The >> ME uses internally most boolean_type_node and IMHO it should be the >> variant used there. As this conversation to a single boolean_type >> (with recast to result FE's boolean type on demand) has some >> advantages on optimization passes. Additionally it simplifies logic >> in passes on types. For example there are some expressions, which are >> in general unexpected in ME as they are transformed in gimplification >> (like TRUTH_ANDIF/ORIF_EXPR). By adding tree manual, you might cause >> the same issue as for the logical-expression showing up now. > > OK, then that's definitely not the case for Ada, so the comment is incorrect. Yes, I will adjust comment here about ADA. Code for ADA looks sane. Just one nit I saw in trans.c, which might be a cause here. >> Well, this patch might be an alternative, but I see here potential >> issues in such none-gimplified expressions for comparision and logical >> not, which not necessariily have BOOLEAN_TYPE. See here the code for >> fold_truth_not (and some other places) in fold-const. So I think, as >> long as we have here external gimplication it is more save to check >> just for integral-kind. > > Note that, even without "external gimplication", we still have integral types > down to the tree-cfg.c check. Take ACATS c52103x at -O0. The Ada FE hands > over a valid TRUTH_AND_EXPR, i.e. (BOOLEAN_TYPE, BOOLEAN_TYPE, BOOLEAN_TYPE) > but the gimplifier builds a (BOOLEAN_TYPE, INTEGER_TYPE, BOOLEAN_TYPE) as it > strips, then adds, then re-strips a cast to BOOLEAN_TYPE in gimplify_expr. With this patch (which would describe why it gimplifier sees integer-type nodes here): Index: gcc/gcc/ada/gcc-interface/trans.c =================================================================== --- gcc.orig/gcc/ada/gcc-interface/trans.c 2011-05-12 20:06:01.000000000 +0200 +++ gcc/gcc/ada/gcc-interface/trans.c 2011-05-15 15:33:32.305516200 +0200 @@ -7101,7 +7110,7 @@ convert_with_check (Entity_Id gnat_type, { /* Ensure GNU_EXPR only gets evaluated once. */ tree gnu_input = gnat_protect_expr (gnu_result); - tree gnu_cond = integer_zero_node; + tree gnu_cond = boolean_false_node; tree gnu_in_lb = TYPE_MIN_VALUE (gnu_in_basetype); tree gnu_in_ub = TYPE_MAX_VALUE (gnu_in_basetype); tree gnu_out_lb = TYPE_MIN_VALUE (gnu_base_type); I was able to do a bootstrap for ada and run 'make check-ada' without seeing gimplification errors. The only failure I see in testrun is 'cxg2001.adb' test with 'GCC error: in compensate_edge, at reg-stach.c:2781' Error detect around cxg2001.adb:322:5. But well, this bug seems to me unrelated here to gimplication. But maybe I am wrong here. Regards, Kai
2011/5/15 Kai Tietz <ktietz70@googlemail.com>: > 2011/5/15 Eric Botcazou <ebotcazou@adacore.com>: >>> Well, I mean by artificial here, that gimplification is done via >>> gimplify_expr API. As FE and ME have here different assumptions. The >>> ME uses internally most boolean_type_node and IMHO it should be the >>> variant used there. As this conversation to a single boolean_type >>> (with recast to result FE's boolean type on demand) has some >>> advantages on optimization passes. Additionally it simplifies logic >>> in passes on types. For example there are some expressions, which are >>> in general unexpected in ME as they are transformed in gimplification >>> (like TRUTH_ANDIF/ORIF_EXPR). By adding tree manual, you might cause >>> the same issue as for the logical-expression showing up now. >> >> OK, then that's definitely not the case for Ada, so the comment is incorrect. > > Yes, I will adjust comment here about ADA. Code for ADA looks sane. > Just one nit I saw in trans.c, which might be a cause here. > >>> Well, this patch might be an alternative, but I see here potential >>> issues in such none-gimplified expressions for comparision and logical >>> not, which not necessariily have BOOLEAN_TYPE. See here the code for >>> fold_truth_not (and some other places) in fold-const. So I think, as >>> long as we have here external gimplication it is more save to check >>> just for integral-kind. >> >> Note that, even without "external gimplication", we still have integral types >> down to the tree-cfg.c check. Take ACATS c52103x at -O0. The Ada FE hands >> over a valid TRUTH_AND_EXPR, i.e. (BOOLEAN_TYPE, BOOLEAN_TYPE, BOOLEAN_TYPE) >> but the gimplifier builds a (BOOLEAN_TYPE, INTEGER_TYPE, BOOLEAN_TYPE) as it >> strips, then adds, then re-strips a cast to BOOLEAN_TYPE in gimplify_expr. > > With this patch (which would describe why it gimplifier sees > integer-type nodes here): > > Index: gcc/gcc/ada/gcc-interface/trans.c > =================================================================== > --- gcc.orig/gcc/ada/gcc-interface/trans.c 2011-05-12 > 20:06:01.000000000 +0200 > +++ gcc/gcc/ada/gcc-interface/trans.c 2011-05-15 15:33:32.305516200 +0200 > @@ -7101,7 +7110,7 @@ convert_with_check (Entity_Id gnat_type, > { > /* Ensure GNU_EXPR only gets evaluated once. */ > tree gnu_input = gnat_protect_expr (gnu_result); > - tree gnu_cond = integer_zero_node; > + tree gnu_cond = boolean_false_node; > tree gnu_in_lb = TYPE_MIN_VALUE (gnu_in_basetype); > tree gnu_in_ub = TYPE_MAX_VALUE (gnu_in_basetype); > tree gnu_out_lb = TYPE_MIN_VALUE (gnu_base_type); > > I was able to do a bootstrap for ada and run 'make check-ada' without > seeing gimplification errors. > > The only failure I see in testrun is 'cxg2001.adb' test with 'GCC > error: in compensate_edge, at reg-stach.c:2781' Error detect around > cxg2001.adb:322:5. But well, this bug seems to me unrelated here to > gimplication. But maybe I am wrong here. > > Regards, > Kai PS: There are more places to fix, I will sent tomorrow a full patch for this after bootstrap and testsuite-run was completetly successful. I saw in later gnat.dg the described error (but no more the truth-not issue). So I was a bit to early to post here. Regards, Kai
On Sat, May 14, 2011 at 6:14 PM, Eric Botcazou <ebotcazou@adacore.com> wrote: >> Those issues should be fixed by the attached patch, which relaxes >> strictness of logical operations in tree-cfg.c file. > > Thanks. > >> 2011-05-14 Kai Tietz >> >> * tree-cfg.c (verify_gimple_assign_unary): Don't enforce >> boolean_type_node >> compatible lhs/rhs types for logical expression. >> (verify_gimple_assign_binary): Likewise. > > - /* We allow only boolean typed or compatible argument and result. */ > - if (!useless_type_conversion_p (boolean_type_node, rhs1_type) > - || !useless_type_conversion_p (boolean_type_node, rhs2_type) > - || !useless_type_conversion_p (boolean_type_node, lhs_type)) > + /* The gimplify code ensures that just boolean_type_node types > + are present, but ADA and FORTRAN using artificial gimple generation > + code which is producing non-gimplify compatible trees. So we need > + to allow here any kind of integral typed argument and result. */ > + if (!INTEGRAL_TYPE_P (rhs1_type) > + || !INTEGRAL_TYPE_P (rhs2_type) > + || !INTEGRAL_TYPE_P (lhs_type)) > > What does "artificial gimple generation code" mean exactly? The only thing > different in Ada is the definition of boolean_type_node which isn't compatible > with the C definition (its precision is 8 instead of 1). That's all. > > For Ada, you can just do: > > Index: tree-cfg.c > =================================================================== > --- tree-cfg.c (revision 173756) > +++ tree-cfg.c (working copy) > @@ -3350,7 +3350,7 @@ verify_gimple_assign_unary (gimple stmt) > return false; > > case TRUTH_NOT_EXPR: > - if (!useless_type_conversion_p (boolean_type_node, rhs1_type)) > + if (TREE_CODE (rhs1_type) != BOOLEAN_TYPE) > { > error ("invalid types in truth not"); > debug_generic_expr (lhs_type); > @@ -3558,10 +3558,10 @@ do_pointer_plus_expr_check: > case TRUTH_OR_EXPR: > case TRUTH_XOR_EXPR: > { > - /* We allow only boolean typed or compatible argument and result. */ > - if (!useless_type_conversion_p (boolean_type_node, rhs1_type) > - || !useless_type_conversion_p (boolean_type_node, rhs2_type) > - || !useless_type_conversion_p (boolean_type_node, lhs_type)) > + /* We allow only boolean-typed argument and result. */ > + if (TREE_CODE (rhs1_type) != BOOLEAN_TYPE > + || TREE_CODE (rhs2_type) != BOOLEAN_TYPE > + || TREE_CODE (lhs_type) != BOOLEAN_TYPE) > { > error ("type mismatch in binary truth expression"); > debug_generic_expr (lhs_type); We can't use a test for BOOLEAN_TYPE as the middle-end considers a INTEGER_TYPE with same precision/signedness as compatible and thus may propagate a variable of INTEGER_TYPE there. I don't understand why promoting bools to boolean_type_node during gimplification does not work (it might be slightly undesirable as it might introduce conversions from one boolean type to another). So, to relax the above check to allow any of the boolean types we'd need a way to enumerate them. Or make conversions to/from BOOLEAN_TYPEs not useless. Kai - your testing should have catched the Ada testsuite fail, please inverstigate why your setup didn't catch it. Richard.
Hi, On Mon, 16 May 2011, Richard Guenther wrote: > We can't use a test for BOOLEAN_TYPE as the middle-end considers a > INTEGER_TYPE with same precision/signedness as compatible and thus may > propagate a variable of INTEGER_TYPE there. I don't understand why > promoting bools to boolean_type_node during gimplification does not work > (it might be slightly undesirable as it might introduce conversions from > one boolean type to another). > > So, to relax the above check to allow any of the boolean types we'd need > a way to enumerate them. Or make conversions to/from BOOLEAN_TYPEs not > useless. I think conversion _to_ BOOLEAN_TYPE shouldn't be useless, on the grounds that it requires booleanization (at least conceptually), i.e. conversion to a set of two values (no matter the precision or size) based on the outcome of comparing the RHS value with false_pre_image(TREE_TYPE(RHS)). Conversion _from_ BOOLEAN_TYPE can be regarded as useless, as the conversions from false or true into false_pre_image or true_pre_image always is simply an embedding of 0 or 1/-1 (depending on target type signedness). And if the BOOLEAN_TYPE and the LHS have same signedness the bit representation of boolean_true_type is (or should be) the same as the one converted to LHS (namely either 1 or -1). Ciao, Michael.
On Mon, May 16, 2011 at 3:17 PM, Michael Matz <matz@suse.de> wrote: > Hi, > > On Mon, 16 May 2011, Richard Guenther wrote: > >> We can't use a test for BOOLEAN_TYPE as the middle-end considers a >> INTEGER_TYPE with same precision/signedness as compatible and thus may >> propagate a variable of INTEGER_TYPE there. I don't understand why >> promoting bools to boolean_type_node during gimplification does not work >> (it might be slightly undesirable as it might introduce conversions from >> one boolean type to another). >> >> So, to relax the above check to allow any of the boolean types we'd need >> a way to enumerate them. Or make conversions to/from BOOLEAN_TYPEs not >> useless. > > I think conversion _to_ BOOLEAN_TYPE shouldn't be useless, on the grounds > that it requires booleanization (at least conceptually), i.e. conversion > to a set of two values (no matter the precision or size) based on the > outcome of comparing the RHS value with false_pre_image(TREE_TYPE(RHS)). > > Conversion _from_ BOOLEAN_TYPE can be regarded as useless, as the > conversions from false or true into false_pre_image or true_pre_image > always is simply an embedding of 0 or 1/-1 (depending on target type > signedness). And if the BOOLEAN_TYPE and the LHS have same signedness the > bit representation of boolean_true_type is (or should be) the same as the > one converted to LHS (namely either 1 or -1). Sure, that would probably be enough to prevent non-BOOLEAN_TYPEs be used where BOOLEAN_TYPE nodes were used before. It still will cause an artificial conversion from a single-bit bitfield read to a bool. Richard. > > Ciao, > Michael. >
Hi, On Mon, 16 May 2011, Richard Guenther wrote: > > I think conversion _to_ BOOLEAN_TYPE shouldn't be useless, on the > > grounds that it requires booleanization (at least conceptually), i.e. > > conversion to a set of two values (no matter the precision or size) > > based on the outcome of comparing the RHS value with > > false_pre_image(TREE_TYPE(RHS)). > > > > Conversion _from_ BOOLEAN_TYPE can be regarded as useless, as the > > conversions from false or true into false_pre_image or true_pre_image > > always is simply an embedding of 0 or 1/-1 (depending on target type > > signedness). And if the BOOLEAN_TYPE and the LHS have same signedness > > the bit representation of boolean_true_type is (or should be) the same > > as the one converted to LHS (namely either 1 or -1). > > Sure, that would probably be enough to prevent non-BOOLEAN_TYPEs be used > where BOOLEAN_TYPE nodes were used before. It still will cause an > artificial conversion from a single-bit bitfield read to a bool. Not if you're special casing single-bit conversions (on the grounds that a booleanization from two-valued set to a different two-valued set of the same signedness will not actually require a comparison). I think it's better to be very precise in our base predicates than to add various hacks over the place to care for imprecision. Ciao, Michael.
On Mon, May 16, 2011 at 3:45 PM, Michael Matz <matz@suse.de> wrote: > Hi, > > On Mon, 16 May 2011, Richard Guenther wrote: > >> > I think conversion _to_ BOOLEAN_TYPE shouldn't be useless, on the >> > grounds that it requires booleanization (at least conceptually), i.e. >> > conversion to a set of two values (no matter the precision or size) >> > based on the outcome of comparing the RHS value with >> > false_pre_image(TREE_TYPE(RHS)). >> > >> > Conversion _from_ BOOLEAN_TYPE can be regarded as useless, as the >> > conversions from false or true into false_pre_image or true_pre_image >> > always is simply an embedding of 0 or 1/-1 (depending on target type >> > signedness). And if the BOOLEAN_TYPE and the LHS have same signedness >> > the bit representation of boolean_true_type is (or should be) the same >> > as the one converted to LHS (namely either 1 or -1). >> >> Sure, that would probably be enough to prevent non-BOOLEAN_TYPEs be used >> where BOOLEAN_TYPE nodes were used before. It still will cause an >> artificial conversion from a single-bit bitfield read to a bool. > > Not if you're special casing single-bit conversions (on the grounds that a > booleanization from two-valued set to a different two-valued set of > the same signedness will not actually require a comparison). I think it's > better to be very precise in our base predicates than to add various hacks > over the place to care for imprecision. Or require a 1-bit integral type for TRUTH_* operands only (which ensures the two-valueness which is what we really want). That can be done by either fixing the frontends to make boolean_type_node have 1-bit precision or to build a middle-end private type with that constraints (though that's the more difficult route as we still do not have a strong FE - middle-end hand-off point, and it certainly is not the gimplifier). Long term all the global trees should be FE private and the middle-end should have its own set. Richard. > > Ciao, > Michael.
different in Ada is the definition of boolean_type_node which isn't compatible with the C definition (its precision is 8 instead of 1). That's all. For Ada, you can just do: Index: tree-cfg.c =================================================================== --- tree-cfg.c (revision 173756) +++ tree-cfg.c (working copy) @@ -3350,7 +3350,7 @@ verify_gimple_assign_unary (gimple stmt) return false; case TRUTH_NOT_EXPR: - if (!useless_type_conversion_p (boolean_type_node, rhs1_type)) + if (TREE_CODE (rhs1_type) != BOOLEAN_TYPE) { error ("invalid types in truth not"); debug_generic_expr (lhs_type); @@ -3558,10 +3558,10 @@ do_pointer_plus_expr_check: case TRUTH_OR_EXPR: case TRUTH_XOR_EXPR: { - /* We allow only boolean typed or compatible argument and result. */ - if (!useless_type_conversion_p (boolean_type_node, rhs1_type) - || !useless_type_conversion_p (boolean_type_node, rhs2_type) - || !useless_type_conversion_p (boolean_type_node, lhs_type)) + /* We allow only boolean-typed argument and result. */ + if (TREE_CODE (rhs1_type) != BOOLEAN_TYPE + || TREE_CODE (rhs2_type) != BOOLEAN_TYPE + || TREE_CODE (lhs_type) != BOOLEAN_TYPE) { error ("type mismatch in binary truth expression"); debug_generic_expr (lhs_type);