On Thu, 14 Dec 2017, Jakub Jelinek wrote:
> Hi!
>
> The following testcase FAILs -fcompare-debug, because one COND_EXPR
> branch from the FE during gimplifications is just <nop_expr <integer_cst>>
> which doesn't have TREE_SIDE_EFFECTS, but for -gstatement-frontiers it
> is a STATEMENT_LIST which contains # DEBUG BEGIN_STMT and that <nop_expr
> <integer_cst>>.
Ugh... the issue is that this difference might have many other
-fcompare-debug issues, like when folding things? Why is it a
STATEMENT_LIST rather than a COMPOUND_EXPR?
> Neither # DEBUG BEGIN_STMT nor that NOP_EXPR have
> TREE_SIDE_EFFECTS, but STATEMENT_LIST has TREE_SIDE_EFFECTS already from
> make_node and the gimplifier (and apparently the C++ FE too) checks
> just that bit. With { { { 0; } { 1; } { 2; } { 3; } } } one can end up
> with quite large STATEMENT_LIST subtrees which in reality still don't
> have any side-effects.
> Maintaining accurate TREE_SIDE_EFFECTS bit on STATEMENT_LIST would be hard,
> if we would only add and never remove statements, then we could just clear
> it during make_node and set it whenever adding TREE_SIDE_EFFECTS statement
> into it, but as soon as we sometimes remove from STATEMENT_LIST, or merge
> STATEMENT_LISTs etc., maintaining this is too IMHO expensive, especially
> when we usually just will not care about it.
> So, I think it is better to just compute this lazily in the few spots where
> we are interested about this, in real-world testcases most of the
> STATEMENT_LISTs will have side-effects and should find them pretty early
> when walking the tree.
> As a side-effect, this patch will handle those
> { { { 0; } { 1; } { 2; } { 3; } } } and similar then/else statement lists
> better.
I don't like this too much. Iff then we should do "real" lazy
computation, like adding a TREE_SIDE_EFFECTS_VALID flag on STATEMENT_LIST,
keeping TREE_SIDE_EFFECTS up-to-date when easily possible and when doing
the expensive thing cache the result. That said, I'm not convinced
this will fix -fcompare-debug issues for good. Is it really necessary
to introduce this IL difference so early and in such an intrusive way?
Can't we avoid adding # DEBUG BEGIN_STMT when there's not already
a STATEMENT_LIST around for example?
Thanks,
Richard.
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
>
> 2017-12-14 Jakub Jelinek <ja...@redhat.com>
>
> PR debug/83419
> * tree.h (statement_with_side_effects_p): Declare.
> * tree.c (statement_with_side_effects_p): New function.
> * gimplify.c (shortcut_cond_expr, gimplify_cond_expr): Use it.
>
> * cp-gimplify.c (genericize_if_stmt, gimplify_expr_stmt,
> cp_fold): Use statement_with_side_effects_p instead of
> just TREE_SIDE_EFFECTS.
>
> * gcc.dg/pr83419.c: New test.
>
> --- gcc/tree.h.jj 2017-12-12 09:48:15.000000000 +0100
> +++ gcc/tree.h 2017-12-14 13:22:34.157858781 +0100
> @@ -4780,6 +4780,7 @@ extern tree obj_type_ref_class (const_tr
> extern bool types_same_for_odr (const_tree type1, const_tree type2,
> bool strict=false);
> extern bool contains_bitfld_component_ref_p (const_tree);
> +extern bool statement_with_side_effects_p (tree);
> extern bool block_may_fallthru (const_tree);
> extern void using_eh_for_cleanups (void);
> extern bool using_eh_for_cleanups_p (void);
> --- gcc/tree.c.jj 2017-12-12 09:48:15.000000000 +0100
> +++ gcc/tree.c 2017-12-14 13:21:25.857752480 +0100
> @@ -12296,6 +12296,26 @@ contains_bitfld_component_ref_p (const_t
> return false;
> }
>
> +/* Return true if STMT has side-effects. This is like
> + TREE_SIDE_EFFECTS (stmt), except it returns false for NULL and if STMT
> + is a STATEMENT_LIST, it recurses on the statements. */
> +
> +bool
> +statement_with_side_effects_p (tree stmt)
> +{
> + if (stmt == NULL_TREE)
> + return false;
> + if (TREE_CODE (stmt) != STATEMENT_LIST)
> + return TREE_SIDE_EFFECTS (stmt);
> +
> + for (tree_stmt_iterator i = tsi_start (stmt);
> + !tsi_end_p (i); tsi_next (&i))
> + if (statement_with_side_effects_p (tsi_stmt (i)))
> + return true;
> +
> + return false;
> +}
> +
> /* Try to determine whether a TRY_CATCH expression can fall through.
> This is a subroutine of block_may_fallthru. */
>
> --- gcc/gimplify.c.jj 2017-12-14 11:53:34.907142223 +0100
> +++ gcc/gimplify.c 2017-12-14 13:18:19.261184074 +0100
> @@ -3637,8 +3637,8 @@ shortcut_cond_expr (tree expr)
> tree *true_label_p;
> tree *false_label_p;
> bool emit_end, emit_false, jump_over_else;
> - bool then_se = then_ && TREE_SIDE_EFFECTS (then_);
> - bool else_se = else_ && TREE_SIDE_EFFECTS (else_);
> + bool then_se = statement_with_side_effects_p (then_);
> + bool else_se = statement_with_side_effects_p (else_);
>
> /* First do simple transformations. */
> if (!else_se)
> @@ -3656,7 +3656,7 @@ shortcut_cond_expr (tree expr)
> if (rexpr_has_location (pred))
> SET_EXPR_LOCATION (expr, rexpr_location (pred));
> then_ = shortcut_cond_expr (expr);
> - then_se = then_ && TREE_SIDE_EFFECTS (then_);
> + then_se = statement_with_side_effects_p (then_);
> pred = TREE_OPERAND (pred, 0);
> expr = build3 (COND_EXPR, void_type_node, pred, then_, NULL_TREE);
> SET_EXPR_LOCATION (expr, locus);
> @@ -3678,7 +3678,7 @@ shortcut_cond_expr (tree expr)
> if (rexpr_has_location (pred))
> SET_EXPR_LOCATION (expr, rexpr_location (pred));
> else_ = shortcut_cond_expr (expr);
> - else_se = else_ && TREE_SIDE_EFFECTS (else_);
> + else_se = statement_with_side_effects_p (else_);
> pred = TREE_OPERAND (pred, 0);
> expr = build3 (COND_EXPR, void_type_node, pred, NULL_TREE, else_);
> SET_EXPR_LOCATION (expr, locus);
> @@ -3982,9 +3982,9 @@ gimplify_cond_expr (tree *expr_p, gimple
> if (gimplify_ctxp->allow_rhs_cond_expr
> /* If either branch has side effects or could trap, it can't be
> evaluated unconditionally. */
> - && !TREE_SIDE_EFFECTS (then_)
> + && !statement_with_side_effects_p (then_)
> && !generic_expr_could_trap_p (then_)
> - && !TREE_SIDE_EFFECTS (else_)
> + && !statement_with_side_effects_p (else_)
> && !generic_expr_could_trap_p (else_))
> return gimplify_pure_cond_expr (expr_p, pre_p);
>
> --- gcc/cp/cp-gimplify.c.jj 2017-12-05 10:16:48.000000000 +0100
> +++ gcc/cp/cp-gimplify.c 2017-12-14 13:24:08.373614768 +0100
> @@ -176,9 +176,9 @@ genericize_if_stmt (tree *stmt_p)
> if (!else_)
> else_ = build_empty_stmt (locus);
>
> - if (integer_nonzerop (cond) && !TREE_SIDE_EFFECTS (else_))
> + if (integer_nonzerop (cond) && !statement_with_side_effects_p (else_))
> stmt = then_;
> - else if (integer_zerop (cond) && !TREE_SIDE_EFFECTS (then_))
> + else if (integer_zerop (cond) && !statement_with_side_effects_p (then_))
> stmt = else_;
> else
> stmt = build3 (COND_EXPR, void_type_node, cond, then_, else_);
> @@ -424,7 +424,7 @@ gimplify_expr_stmt (tree *stmt_p)
> gimplification. */
> if (stmt && warn_unused_value)
> {
> - if (!TREE_SIDE_EFFECTS (stmt))
> + if (!statement_with_side_effects_p (stmt))
> {
> if (!IS_EMPTY_STMT (stmt)
> && !VOID_TYPE_P (TREE_TYPE (stmt))
> @@ -2096,7 +2096,7 @@ cp_fold (tree x)
> /* Strip CLEANUP_POINT_EXPR if the expression doesn't have side
> effects. */
> r = cp_fold_rvalue (TREE_OPERAND (x, 0));
> - if (!TREE_SIDE_EFFECTS (r))
> + if (!statement_with_side_effects_p (r))
> x = r;
> break;
>
> --- gcc/testsuite/gcc.dg/pr83419.c.jj 2017-12-14 13:14:45.520969384 +0100
> +++ gcc/testsuite/gcc.dg/pr83419.c 2017-12-14 13:14:45.520969384 +0100
> @@ -0,0 +1,16 @@
> +/* PR debug/83419 */
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -fcompare-debug" } */
> +
> +int a, b;
> +void foo (int, ...);
> +
> +void
> +bar (void)
> +{
> + if (a || 1 == b)
> + foo (1);
> + else
> + 0;
> + foo (1, 0);
> +}
>
> Jakub
>
>
--
Richard Biener <rguent...@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB
21284 (AG Nuernberg)
