Re: [patch gimplify]: Make sure comparison using boolean-type after gimplification

[Kai Tietz <ktietz70 at googlemail dot com>]

2011/5/26 Richard Guenther <richard.guenther@gmail.com>:
> On Thu, May 26, 2011 at 12:20 PM, Kai Tietz <ktietz@redhat.com> wrote:
>> Hello,
>>
>> this patch ensures that after gimplification also comparison expressions using FE's boolean_type_node. ?As we need to deal here with C/C++'s (obj-c/c++ and java's), Ada's, and Fortran's specific boolean types, this patch alters some checks in tree-cfg for Ada's sake, and we need to deal in fold-const about type-conversion of comparisons special.
>> Additionally it takes care that in forwprop pass we don't do type hoising for boolean types.
>>
>> ChangeLog
>>
>> 2011-05-26 ?Kai Tietz
>>
>> ? ? ? ? ?* gimplify.c (gimple_boolify): Boolify all comparison
>> ? ? ? ? ?expressions.
>> ? ? ? ? ?(gimplify_expr): Use 'useless_type_conversion_p' for comparing
>> ? ? ? ? ?org_type with boolean_type_node for TRUTH-expressions and comparisons.
>> ? ? ? ? ?* fold-const.c (fold_unary_loc): Handle comparison conversions with
>> ? ? ? ? ?boolean-type special.
>> ? ? ? ? ?* tree-cfg.c (verify_gimple_comparison): Adjust check for boolean
>> ? ? ? ? ?or compatible types.
>> ? ? ? ? ?(verify_gimple_assign_unary): Likewise.
>> ? ? ? ? ?* tree-ssa-forwprop.c (forward_propagate_comparison): Handle
>> ? ? ? ? ?boolean case special.
>>
>> Tested on x86_64-pc-linux-gnu (multilib) with regression test for all standard languages (C, C++, Obj-C, Fortran, Java) plus Obj-C++ and Ada. Ok for apply?
>
> It obviously isn't ok to apply before a patch has gone in that will resolve
> all of the FAILs you specify. ?Comments on the patch:
>
> @@ -7281,9 +7284,28 @@ gimplify_expr (tree *expr_p, gimple_seq
> ? ? ? ? ? ? ? ? plain wrong if bitfields are involved. ?*/
> ? ? ? ? ? ? ? ?{
> ? ? ? ? ? ? ? ? ?tree type = TREE_TYPE (TREE_OPERAND (*expr_p, 1));
> + ? ? ? ? ? ? ? ? tree org_type = TREE_TYPE (*expr_p);
> +
> + ? ? ? ? ? ? ? ? if (!useless_type_conversion_p (org_type, boolean_type_node))
> + ? ? ? ? ? ? ? ? ? {
> + ? ? ? ? ? ? ? ? ? ? TREE_TYPE (*expr_p) = boolean_type_node;
> + ? ? ? ? ? ? ? ? ? ? *expr_p = fold_convert_loc (saved_location, org_type, *expr_p);
> + ? ? ? ? ? ? ? ? ? ? ret = GS_OK;
> + ? ? ? ? ? ? ? ? ? ? goto dont_recalculate;
> + ? ? ? ? ? ? ? ? ? }
>
> The above should be only done for !AGGREGATE_TYPE_P. ?Probably then
> the strange dont_recalcuate goto can go away as well.

I thought so too, but boolification is required for all kind of
comparisons.  This goto is a bit hacky, but the best way to prevent
here an useless recalculation. Without doing this, we get
bootstrap/regression test failures, as in tree-cfg we check that
comparison type is of boolean (or compatible kind).

> ? ? ? ? ? ? ? ? ?if (!AGGREGATE_TYPE_P (type))
> - ? ? ? ? ? ? ? ? ? goto expr_2;
> + ? ? ? ? ? ? ? ? ? {
> + ? ? ? ? ? ? ? ? ? ? enum gimplify_status r0, r1;
> +
> + ? ? ? ? ? ? ? ? ? ? r0 = gimplify_expr (&TREE_OPERAND (*expr_p, 0), pre_p,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? post_p, is_gimple_val, fb_rvalue);
> + ? ? ? ? ? ? ? ? ? ? r1 = gimplify_expr (&TREE_OPERAND (*expr_p, 1), pre_p,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? post_p, is_gimple_val, fb_rvalue);
> +
> + ? ? ? ? ? ? ? ? ? ? ret = MIN (r0, r1);
> + ? ? ? ? ? ? ? ? ? }
> +
>
> why change this?

This part can be reverted to the goto expr_2 again.

> @@ -7641,6 +7641,12 @@ fold_unary_loc (location_t loc, enum tre
> ? ? ? ?}
> ? ? ? else if (COMPARISON_CLASS_P (arg0))
> ? ? ? ?{
> + ? ? ? ? /* Don't optimize type change, if op0 is of kind boolean_type_node.
> + ? ? ? ? ? ?Otherwise this will lead to race-condition on gimplification
> + ? ? ? ? ? ?trying to boolify comparison expression. ?*/
> + ? ? ? ? if (TREE_TYPE (op0) == boolean_type_node)
> + ? ? ? ? ? return NULL_TREE;
> +
> ? ? ? ? ?if (TREE_CODE (type) == BOOLEAN_TYPE)
> ? ? ? ? ? ?{
> ? ? ? ? ? ? ?arg0 = copy_node (arg0);
>
> The code leading here looks quite strange to me ...

Issue is that Fortran (and this is the cause for this
boolean_type_node check) has more then one boolean type.  Gimplifier
normalizes to boolean_type_node.  If now a different boolean-type
(with different mode) of Fortran is used, it leads to an endless-loop
in type conversion. An example boolean-kind(mode-size=1) and
boolean-kind(mode-size=2) are no useless type conversion. So by
gimplifier type gets set to boolean-kind(mode-size=1) (if this is
default boolean_type_node's definition), and then casted to
boolean-kind(mode-size=2) expression.


> tree
> fold_unary_loc (location_t loc, enum tree_code code, tree type, tree op0)
> {
> ...
> ?if (TREE_CODE_CLASS (code) == tcc_unary)
> ? ?{
> ...
> ? ? ?else if (COMPARISON_CLASS_P (arg0))
> ? ? ? ?{
> ? ? ? ? ?if (TREE_CODE (type) == BOOLEAN_TYPE)
> ? ? ? ? ? ?{
> ? ? ? ? ? ? ?arg0 = copy_node (arg0);
> ? ? ? ? ? ? ?TREE_TYPE (arg0) = type;
> ? ? ? ? ? ? ?return arg0;
> ? ? ? ? ? ?}
> ? ? ? ? ?else if (TREE_CODE (type) != INTEGER_TYPE)
> ? ? ? ? ? ?return fold_build3_loc (loc, COND_EXPR, type, arg0,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?fold_build1_loc (loc, code, type,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? integer_one_node),
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?fold_build1_loc (loc, code, type,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? integer_zero_node));
> ? ? ? ?}
>
> so, for any tcc_unary, like NEGATE_EXPR, with BOOLEAN_TYPE,
> return arg0 ... sure. ?Same for the 2nd case. ?~ (a == b) isn't
> the same as a == b ? ~1 : ~0. ?I _suppose_ those cases were
> ment for CONVERT_EXPR_CODE_P (code) instead of all of tcc_unary,
> in which case they should be dropped or moved below where we
> handle conversions explicitly.
>
> That said - does anyone remember anything about that above code?
> Trying to do some svn blame history tracking now ...
>
> Richard.
>
>> The following tests are failing due this change (what is to be expected here and needs some additional handling in forwprop).
>> FAIL: gcc.dg/binop-xor1.c scan-tree-dump-times optimized "<bb[^>]*>" 5
>> FAIL: gcc.dg/binop-xor1.c scan-tree-dump-times optimized "\^" 1
>> FAIL: gcc.dg/binop-xor3.c scan-tree-dump-times optimized "<bb[^>]*>" 1
>> FAIL: gcc.dg/binop-xor3.c scan-tree-dump-times optimized "\^" 1
>> XPASS: gcc.dg/tree-ssa/20030807-7.c scan-tree-dump-times vrp1 "if " 1
>> FAIL: gcc.dg/tree-ssa/builtin-expect-5.c scan-tree-dump forwprop1 "builtin_expect[^\n]*, 1\);\n[^\n]*if"
>> FAIL: gcc.dg/tree-ssa/pr21031.c scan-tree-dump-times forwprop1 "Replaced" 2
>> FAIL: gcc.dg/tree-ssa/pr30978.c scan-tree-dump optimized "e_. = a_..D. > 0;"
>> FAIL: gcc.dg/tree-ssa/ssa-fre-6.c scan-tree-dump-times fre1 "Replaced " 5
>> FAIL: gcc.dg/tree-ssa/vrp47.c scan-tree-dump-times dom1 "x[^ ]* & y" 1
>> FAIL: gcc.dg/tree-ssa/vrp47.c scan-tree-dump-times vrp1 "x[^ ]* \^ 1" 1
>> FAIL: gcc.dg/vect/pr49038.c execution test
>> FAIL: gcc.dg/vect/pr49038.c -flto execution test
>> FAIL: gcc.target/i386/andor-2.c scan-assembler-not sete
>>
>> The Ada, Obj-C, Obj-C++, C++, Fortran, and Java testsuite don't show any new regressions by this.
>>
>> Some failing testcases are simply caused by different folding behavior and producing simplier code. ?The binop-xor tests 1 and 3 might be better removed for now, or marked as being expect to fail. The cause for their failing is in doing tree-analysis via fold-const on gimplified trees, which now don't allow folding here to look through.
>> To illustrate required changes for other tests, I attach here some required changes for testsuite
>>
>> Index: gcc.dg/tree-ssa/pr30978.c
>> ===================================================================
>> --- gcc.dg/tree-ssa/pr30978.c ? (revision 174264)
>> +++ gcc.dg/tree-ssa/pr30978.c ? (working copy)
>> @@ -10,5 +10,5 @@
>> ? return e;
>> ?}
>>
>> -/* { dg-final { scan-tree-dump "e_. = a_..D. > 0;" "optimized" } } */
>> +/* { dg-final { scan-tree-dump " = a_..D. > 0;\n[^\n]*e_. = \\\(int\\\)" "optimized" } } */
>> ?/* { dg-final { cleanup-tree-dump "optimized" } } */
>>
>> Index: gcc.dg/tree-ssa/builtin-expect-5.c
>> ===================================================================
>> --- gcc.dg/tree-ssa/builtin-expect-5.c ?(revision 174264)
>> +++ gcc.dg/tree-ssa/builtin-expect-5.c ?(working copy)
>> @@ -11,5 +11,5 @@
>>
>> ?/* { dg-final { scan-tree-dump-times { if } 2 "forwprop1"} } */
>> ?/* { dg-final { scan-tree-dump {builtin_expect[^\n]*, 0\);\n[^\n]*if} "forwprop1"} } */
>> -/* { dg-final { scan-tree-dump {builtin_expect[^\n]*, 1\);\n[^\n]*if} "forwprop1"} } */
>> +/* { dg-final { scan-tree-dump {builtin_expect[^\n]*, 1\);} "forwprop1"} } */
>> ?/* { dg-final { cleanup-tree-dump "forwprop?" } } */
>>
>> Index: gcc.dg/tree-ssa/pr21031.c
>> ===================================================================
>> --- gcc.dg/tree-ssa/pr21031.c ? (revision 174264)
>> +++ gcc.dg/tree-ssa/pr21031.c ? (working copy)
>> @@ -16,5 +16,5 @@
>> ? ? return 0;
>> ?}
>>
>> -/* { dg-final { scan-tree-dump-times "Replaced" 2 "forwprop1"} } */
>> +/* { dg-final { scan-tree-dump-times "Replaced" 1 "forwprop1"} } */
>> ?/* { dg-final { cleanup-tree-dump "forwprop1" } } */
>>
>> Index: gcc.dg/tree-ssa/ssa-fre-6.c
>> ===================================================================
>> --- gcc.dg/tree-ssa/ssa-fre-6.c (revision 174264)
>> +++ gcc.dg/tree-ssa/ssa-fre-6.c (working copy)
>> @@ -2,5 +2,5 @@
>> ?/* { dg-options "-O -fdump-tree-fre1-details" } */
>>
>> ?int i; int foo(void) { i = 2; int j = i * 2; int k = i + 2; return j == k; }
>> -/* { dg-final { scan-tree-dump-times "Replaced " 5 "fre1" } } */
>> +/* { dg-final { scan-tree-dump-times "Replaced " 6 "fre1" } } */
>> ?/* { dg-final { cleanup-tree-dump "fre1" } } */
>>
>> Regards,
>> Kai
>>

Regards,
Kai