Re: [patch tree-optimization]: [2 of 3]: Boolify compares & more

[Richard Guenther <richard dot guenther at gmail dot com>]

On Thu, Jul 21, 2011 at 1:12 PM, Kai Tietz <ktietz70@googlemail.com> wrote:
> Hello,
>
> Updated and again merged variant of this patch.
> We have now just two vrp related regressions by this patch,
> and it is addressed already by a posted one.
>
> 2011-07-21 ?Kai Tietz ?<ktietz@redhat.com>
>
> ? ? ? ?* fold-const.c (fold_unary_loc): Preserve indirect
> ? ? ? ?comparison cast to none-boolean type.
> ? ? ? ?* tree-ssa.c (useless_type_conversion_p): Preserve cast
> ? ? ? ?from/to boolean-type.
> ? ? ? ?* gimplify.c (gimple_boolify): Handle boolification
> ? ? ? ?of comparisons.
> ? ? ? ?(gimplify_expr): Boolifiy non aggregate-typed
> ? ? ? ?comparisons.
> ? ? ? ?* tree-cfg.c (verify_gimple_comparison): Check result
> ? ? ? ?type of comparison expression.
> ? ? ? ?* tree-ssa-forwprop.c (forward_propagate_comparison):
> ? ? ? ?Adjust test of condition result and disallow type-cast
> ? ? ? ?sinking into comparison.
>
>
> ? ? ? ?* gcc.dg/tree-ssa/pr21031.c: Adjusted.
> ? ? ? ?* gcc.dg/tree-ssa/pr30978.c: Likewise.
> ? ? ? ?* gcc.dg/tree-ssa/ssa-fre-6.c: Likewise.
>
> Bootstrapped and regression tested for x86_64-pc-linux-gnu. ?Ok for apply?

Comments below (well, just two minor testsuite ones).

> Regards,
> Kai
>
> Index: gcc-head/gcc/fold-const.c
> ===================================================================
> --- gcc-head.orig/gcc/fold-const.c
> +++ gcc-head/gcc/fold-const.c
> @@ -7664,11 +7664,11 @@ fold_unary_loc (location_t loc, enum tre
> ? ? ? ? ? ? non-integral type.
> ? ? ? ? ? ? Do not fold the result as that would not simplify further, also
> ? ? ? ? ? ? folding again results in recursions. ?*/
> - ? ? ? ? if (INTEGRAL_TYPE_P (type))
> + ? ? ? ? if (TREE_CODE (type) == BOOLEAN_TYPE)
> ? ? ? ? ? ?return build2_loc (loc, TREE_CODE (op0), type,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? TREE_OPERAND (op0, 0),
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? TREE_OPERAND (op0, 1));
> - ? ? ? ? else
> + ? ? ? ? else if (!INTEGRAL_TYPE_P (type))
> ? ? ? ? ? ?return build3_loc (loc, COND_EXPR, type, op0,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? fold_convert (type, boolean_true_node),
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? fold_convert (type, boolean_false_node));
> Index: gcc-head/gcc/tree-ssa.c
> ===================================================================
> --- gcc-head.orig/gcc/tree-ssa.c
> +++ gcc-head/gcc/tree-ssa.c
> @@ -1306,10 +1306,10 @@ useless_type_conversion_p (tree outer_ty
> ? ? ? ? ?|| TYPE_PRECISION (inner_type) != TYPE_PRECISION (outer_type))
> ? ? ? ?return false;
>
> - ? ? ?/* Preserve conversions to BOOLEAN_TYPE if it is not of precision
> - ? ? ? ? one. ?*/
> - ? ? ?if (TREE_CODE (inner_type) != BOOLEAN_TYPE
> - ? ? ? ? && TREE_CODE (outer_type) == BOOLEAN_TYPE
> + ? ? ?/* Preserve conversions to/from BOOLEAN_TYPE if types are not
> + ? ? ? ?of precision one. ?*/
> + ? ? ?if (((TREE_CODE (inner_type) == BOOLEAN_TYPE)
> + ? ? ? ? ?!= (TREE_CODE (outer_type) == BOOLEAN_TYPE))
> ? ? ? ? ?&& TYPE_PRECISION (outer_type) != 1)
> ? ? ? ?return false;
>
> Index: gcc-head/gcc/testsuite/gcc.dg/binop-xor1.c
> ===================================================================
> --- gcc-head.orig/gcc/testsuite/gcc.dg/binop-xor1.c
> +++ gcc-head/gcc/testsuite/gcc.dg/binop-xor1.c
> @@ -7,8 +7,5 @@ foo (int a, int b, int c)
> ? return ((a && !b && c) || (!a && b && c));
> ?}
>
> -/* We expect to see "<bb N>"; confirm that, so that we know to count
> - ? it in the real test. ?*/
> -/* { dg-final { scan-tree-dump-times "<bb\[^>\]*>" 5 "optimized" } } */
> -/* { dg-final { scan-tree-dump-times "\\\^" 1 "optimized" } } */
> +/* { dg-final { scan-tree-dump-times "\\\^" 1 "optimized" { xfail
> *-*-* } } } */
> ?/* { dg-final { cleanup-tree-dump "optimized" } } */
> Index: gcc-head/gcc/testsuite/gcc.dg/binop-xor3.c
> ===================================================================
> --- gcc-head.orig/gcc/testsuite/gcc.dg/binop-xor3.c
> +++ gcc-head/gcc/testsuite/gcc.dg/binop-xor3.c
> @@ -7,8 +7,5 @@ foo (int a, int b)
> ? return ((a && !b) || (!a && b));
> ?}
>
> -/* We expect to see "<bb N>"; confirm that, so that we know to count
> - ? it in the real test. ?*/
> -/* { dg-final { scan-tree-dump-times "<bb\[^>\]*>" 1 "optimized" } } */
> -/* { dg-final { scan-tree-dump-times "\\\^" 1 "optimized" } } */
> +/* { dg-final { scan-tree-dump-times "\\\^" 1 "optimized" { xfail
> *-*-* } } } */
> ?/* { dg-final { cleanup-tree-dump "optimized" } } */
> Index: gcc-head/gcc/tree-ssa-forwprop.c
> ===================================================================
> --- gcc-head.orig/gcc/tree-ssa-forwprop.c
> +++ gcc-head/gcc/tree-ssa-forwprop.c
> @@ -1132,20 +1132,12 @@ forward_propagate_comparison (gimple stm
> ? if (!INTEGRAL_TYPE_P (TREE_TYPE (lhs)))
> ? ? return false;
>
> - ?/* We can propagate the condition into a conversion. ?*/
> - ?if (CONVERT_EXPR_CODE_P (code))
> - ? ?{
> - ? ? ?/* Avoid using fold here as that may create a COND_EXPR with
> - ? ? ? ?non-boolean condition as canonical form. ?*/
> - ? ? ?tmp = build2 (gimple_assign_rhs_code (stmt), TREE_TYPE (lhs),
> - ? ? ? ? ? ? ? ? ? gimple_assign_rhs1 (stmt), gimple_assign_rhs2 (stmt));
> - ? ?}
> ? /* We can propagate the condition into a statement that
> ? ? ?computes the logical negation of the comparison result. ?*/
> - ?else if ((code == BIT_NOT_EXPR
> - ? ? ? ? ? && TYPE_PRECISION (TREE_TYPE (lhs)) == 1)
> - ? ? ? ? ?|| (code == BIT_XOR_EXPR
> - ? ? ? ? ? ? ?&& integer_onep (gimple_assign_rhs2 (use_stmt))))
> + ?if ((code == BIT_NOT_EXPR
> + ? ? ? && TYPE_PRECISION (TREE_TYPE (lhs)) == 1)
> + ? ? ?|| (code == BIT_XOR_EXPR
> + ? ? ? ? && integer_onep (gimple_assign_rhs2 (use_stmt))))
> ? ? {
> ? ? ? tree type = TREE_TYPE (gimple_assign_rhs1 (stmt));
> ? ? ? bool nans = HONOR_NANS (TYPE_MODE (type));
> @@ -1750,6 +1742,7 @@ simplify_bitwise_binary (gimple_stmt_ite
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?arg2));
> ? ? ? tem = make_ssa_name (tem, newop);
> ? ? ? gimple_assign_set_lhs (newop, tem);
> + ? ? ?gimple_set_location (newop, gimple_location (stmt));
> ? ? ? gsi_insert_before (gsi, newop, GSI_SAME_STMT);
> ? ? ? gimple_assign_set_rhs_with_ops_1 (gsi, NOP_EXPR,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?tem, NULL_TREE, NULL_TREE);
> @@ -1779,6 +1772,7 @@ simplify_bitwise_binary (gimple_stmt_ite
> ? ? ? newop = gimple_build_assign_with_ops (code, tem, def1_arg1, def2_arg1);
> ? ? ? tem = make_ssa_name (tem, newop);
> ? ? ? gimple_assign_set_lhs (newop, tem);
> + ? ? ?gimple_set_location (newop, gimple_location (stmt));
> ? ? ? gsi_insert_before (gsi, newop, GSI_SAME_STMT);
> ? ? ? gimple_assign_set_rhs_with_ops_1 (gsi, NOP_EXPR,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?tem, NULL_TREE, NULL_TREE);
> @@ -1807,6 +1801,7 @@ simplify_bitwise_binary (gimple_stmt_ite
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?tem, def1_arg1, arg2);
> ? ? ? tem = make_ssa_name (tem, newop);
> ? ? ? gimple_assign_set_lhs (newop, tem);
> + ? ? ?gimple_set_location (newop, gimple_location (stmt));
> ? ? ? /* Make sure to re-process the new stmt as it's walking upwards. ?*/
> ? ? ? gsi_insert_before (gsi, newop, GSI_NEW_STMT);
> ? ? ? gimple_assign_set_rhs1 (stmt, tem);
> Index: gcc-head/gcc/gimplify.c
> ===================================================================
> --- gcc-head.orig/gcc/gimplify.c
> +++ gcc-head/gcc/gimplify.c
> @@ -2860,18 +2860,23 @@ gimple_boolify (tree expr)
>
> ? ? case TRUTH_NOT_EXPR:
> ? ? ? TREE_OPERAND (expr, 0) = gimple_boolify (TREE_OPERAND (expr, 0));
> - ? ? ?/* FALLTHRU */
>
> - ? ?case EQ_EXPR: case NE_EXPR:
> - ? ?case LE_EXPR: case GE_EXPR: case LT_EXPR: case GT_EXPR:
> ? ? ? /* These expressions always produce boolean results. ?*/
> - ? ? ?TREE_TYPE (expr) = boolean_type_node;
> + ? ? ?if (TREE_CODE (type) != BOOLEAN_TYPE)
> + ? ? ? TREE_TYPE (expr) = boolean_type_node;
> ? ? ? return expr;
>
> ? ? default:
> + ? ? ?if (COMPARISON_CLASS_P (expr))
> + ? ? ? {
> + ? ? ? ? /* There expressions always prduce boolean results. ?*/
> + ? ? ? ? if (TREE_CODE (type) != BOOLEAN_TYPE)
> + ? ? ? ? ? TREE_TYPE (expr) = boolean_type_node;
> + ? ? ? ? return expr;
> + ? ? ? }
> ? ? ? /* Other expressions that get here must have boolean values, but
> ? ? ? ? might need to be converted to the appropriate mode. ?*/
> - ? ? ?if (type == boolean_type_node)
> + ? ? ?if (TREE_CODE (type) == BOOLEAN_TYPE)
> ? ? ? ?return expr;
> ? ? ? return fold_convert_loc (loc, boolean_type_node, expr);
> ? ? }
> @@ -7316,7 +7321,19 @@ gimplify_expr (tree *expr_p, gimple_seq
> ? ? ? ? ? ? ? ? ?tree type = TREE_TYPE (TREE_OPERAND (*expr_p, 1));
>
> ? ? ? ? ? ? ? ? ?if (!AGGREGATE_TYPE_P (type))
> - ? ? ? ? ? ? ? ? ? goto expr_2;
> + ? ? ? ? ? ? ? ? ? {
> + ? ? ? ? ? ? ? ? ? ? tree org_type = TREE_TYPE (*expr_p);
> + ? ? ? ? ? ? ? ? ? ? *expr_p = gimple_boolify (*expr_p);
> + ? ? ? ? ? ? ? ? ? ? if (!useless_type_conversion_p (org_type,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? TREE_TYPE (*expr_p)))
> + ? ? ? ? ? ? ? ? ? ? ? {
> + ? ? ? ? ? ? ? ? ? ? ? ? *expr_p = fold_convert_loc (input_location,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? org_type, *expr_p);
> + ? ? ? ? ? ? ? ? ? ? ? ? ret = GS_OK;
> + ? ? ? ? ? ? ? ? ? ? ? }
> + ? ? ? ? ? ? ? ? ? ? else
> + ? ? ? ? ? ? ? ? ? ? ? goto expr_2;
> + ? ? ? ? ? ? ? ? ? }
> ? ? ? ? ? ? ? ? ?else if (TYPE_MODE (type) != BLKmode)
> ? ? ? ? ? ? ? ? ? ?ret = gimplify_scalar_mode_aggregate_compare (expr_p);
> ? ? ? ? ? ? ? ? ?else
> Index: gcc-head/gcc/testsuite/gcc.dg/tree-ssa/pr21031.c
> ===================================================================
> --- gcc-head.orig/gcc/testsuite/gcc.dg/tree-ssa/pr21031.c
> +++ gcc-head/gcc/testsuite/gcc.dg/tree-ssa/pr21031.c
> @@ -16,5 +16,5 @@ foo (int a)
> ? ? return 0;
> ?}
>
> -/* { dg-final { scan-tree-dump-times "Replaced" 2 "forwprop1"} } */
> +/* { dg-final { scan-tree-dump-times "Replaced" 1 "forwprop1"} } */

I have looked at this change and it shows a regression for which I am
currently testing a patch, so this change will not be necessary.

> ?/* { dg-final { cleanup-tree-dump "forwprop1" } } */
> Index: gcc-head/gcc/testsuite/gcc.dg/tree-ssa/pr30978.c
> ===================================================================
> --- gcc-head.orig/gcc/testsuite/gcc.dg/tree-ssa/pr30978.c
> +++ gcc-head/gcc/testsuite/gcc.dg/tree-ssa/pr30978.c
> @@ -10,5 +10,5 @@ int foo(int a)
> ? return e;
> ?}
>
> -/* { dg-final { scan-tree-dump "e_. = a_..D. > 0;" "optimized" } } */
> +/* { dg-final { scan-tree-dump " = a_..D. > 0;" "optimized" } } */

That no longer tests what the test was trying to do - we were testing
that the comparison is fully propagated to the return value.  Now we
get (an expected) widening to the return type.  So we should test for
something that still makes sure we did the required optimization,
which would for example be

/* We expect exactly two assignments.  */
/* { dg-final { scan-tree-dump-times " = " 2 "optimized" } } */
/* One comparison and one extension to int.  */
/* { dg-final { scan-tree-dump " = a_..D. > 0;" "optimized" } } */
/* { dg-final { scan-tree-dump "e_. = \\\(int\\\)" "optimized" } } */

please change the testcase this way (verifying that my suggestion
indeed works).

With these two changes the patch is ok to commit (it will also
regress gcc.target/i386/andor-2.c but that is an exact duplicate
of the already regressed gcc.dg/tree-ssa/vrp47.c).

Thanks,
Richard.

> ?/* { dg-final { cleanup-tree-dump "optimized" } } */
> Index: gcc-head/gcc/testsuite/gcc.dg/tree-ssa/ssa-fre-6.c
> ===================================================================
> --- gcc-head.orig/gcc/testsuite/gcc.dg/tree-ssa/ssa-fre-6.c
> +++ gcc-head/gcc/testsuite/gcc.dg/tree-ssa/ssa-fre-6.c
> @@ -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" } } */
> Index: gcc-head/gcc/tree-cfg.c
> ===================================================================
> --- gcc-head.orig/gcc/tree-cfg.c
> +++ gcc-head/gcc/tree-cfg.c
> @@ -3203,7 +3203,9 @@ verify_gimple_comparison (tree type, tre
> ? ? ? ?&& (!POINTER_TYPE_P (op0_type)
> ? ? ? ? ? || !POINTER_TYPE_P (op1_type)
> ? ? ? ? ? || TYPE_MODE (op0_type) != TYPE_MODE (op1_type)))
> - ? ? ?|| !INTEGRAL_TYPE_P (type))
> + ? ? ?|| !INTEGRAL_TYPE_P (type)
> + ? ? ?|| (TREE_CODE (type) != BOOLEAN_TYPE
> + ? ? ? ? && TYPE_PRECISION (type) != 1))
> ? ? {
> ? ? ? error ("type mismatch in comparison expression");
> ? ? ? debug_generic_expr (type);
>