Re: Fix PR41454 (dom miscompiles stuff)

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

On Thu, Sep 24, 2009 at 3:32 PM, Michael Matz <matz@suse.de> wrote:
> Hi,
>
> the basic issue is that dom doesn't call update_stmt early enough, but
> instead queues this to when leaving the block. ?That's not going to work
> as leave_block can thread over backedges. ?There are counter-measures to
> not generate endless loops by that but they depends on updated statement
> for the target block (a dom parent in our case, hence not already
> processed by leave_block, hence with modified but not updated statements).
> In particular FOR_EACH_SSA_USE_OPERAND doesn't work with unupdated
> statements.
>
> The fix is obviously to call update_stmt early enough. ?With that we can
> also do away with the stmts_to_rescan set, that since some time wasn't
> doing interesting things anymore except defering update_stmt calls. ?It
> once was used to defer changing virtual ops that are part of the hash
> code, that has to stay the same until avail expressions are removed. ?But
> that can't happen anymore, and even more so even the removal code doesn't
> really seem to rely on this awkward condition.
>
> Fixes the testcase, regstrapping on x86_64-linux in progress (all
> langs+Ada). ?Okay if that passes?

Ok.

As a followup you might want to remove all computations and
returns of "may_have_exposed_new_symbols".  That is from
the times we had multiple virtual operands which could change
by stmt updating.

Richard.

>
> Ciao,
> Michael.
> --
> ? ? ? ?PR tree-optimization/41454
> ? ? ? ?* tree-ssa-dom (stmts_to_rescan): Remove variable.
> ? ? ? ?(tree_ssa_dominator_optimize): Don't allocate and free it.
> ? ? ? ?(dom_opt_leave_block): Don't iterate over it.
> ? ? ? ?(optimize_stmt): Don't defer updating stmts.
>
> testsuite/
> ? ? ? ?* gcc.dg/pr41454.c: New test.
>
> Index: tree-ssa-dom.c
> ===================================================================
> --- tree-ssa-dom.c ? ? ?(revision 152118)
> +++ tree-ssa-dom.c ? ? ?(working copy)
> @@ -127,15 +127,6 @@ DEF_VEC_ALLOC_P(expr_hash_elt_t,heap);
>
> ?static VEC(expr_hash_elt_t,heap) *avail_exprs_stack;
>
> -/* Stack of statements we need to rescan during finalization for newly
> - ? exposed variables.
> -
> - ? Statement rescanning must occur after the current block's available
> - ? expressions are removed from AVAIL_EXPRS. ?Else we may change the
> - ? hash code for an expression and be unable to find/remove it from
> - ? AVAIL_EXPRS. ?*/
> -static VEC(gimple,heap) *stmts_to_rescan;
> -
> ?/* Structure for entries in the expression hash table. ?*/
>
> ?struct expr_hash_elt
> @@ -623,7 +614,6 @@ tree_ssa_dominator_optimize (void)
> ? avail_exprs = htab_create (1024, real_avail_expr_hash, avail_expr_eq, free_expr_hash_elt);
> ? avail_exprs_stack = VEC_alloc (expr_hash_elt_t, heap, 20);
> ? const_and_copies_stack = VEC_alloc (tree, heap, 20);
> - ?stmts_to_rescan = VEC_alloc (gimple, heap, 20);
> ? need_eh_cleanup = BITMAP_ALLOC (NULL);
>
> ? /* Setup callbacks for the generic dominator tree walker. ?*/
> @@ -733,7 +723,6 @@ tree_ssa_dominator_optimize (void)
>
> ? VEC_free (expr_hash_elt_t, heap, avail_exprs_stack);
> ? VEC_free (tree, heap, const_and_copies_stack);
> - ?VEC_free (gimple, heap, stmts_to_rescan);
>
> ? /* Free the value-handle array. ?*/
> ? threadedge_finalize_values ();
> @@ -1773,20 +1762,6 @@ dom_opt_leave_block (struct dom_walk_dat
>
> ? remove_local_expressions_from_table ();
> ? restore_vars_to_original_value ();
> -
> - ?/* If we queued any statements to rescan in this block, then
> - ? ? go ahead and rescan them now. ?*/
> - ?while (VEC_length (gimple, stmts_to_rescan) > 0)
> - ? ?{
> - ? ? ?gimple stmt = VEC_last (gimple, stmts_to_rescan);
> - ? ? ?basic_block stmt_bb = gimple_bb (stmt);
> -
> - ? ? ?if (stmt_bb != bb)
> - ? ? ? break;
> -
> - ? ? ?VEC_pop (gimple, stmts_to_rescan);
> - ? ? ?update_stmt (stmt);
> - ? ?}
> ?}
>
> ?/* Search for redundant computations in STMT. ?If any are found, then
> @@ -2218,6 +2193,8 @@ optimize_stmt (basic_block bb, gimple_st
> ? if (gimple_modified_p (stmt) || modified_p)
> ? ? {
> ? ? ? tree val = NULL;
> +
> + ? ? ?update_stmt_if_modified (stmt);
>
> ? ? ? if (gimple_code (stmt) == GIMPLE_COND)
> ? ? ? ? val = fold_binary_loc (gimple_location (stmt),
> @@ -2238,13 +2215,6 @@ optimize_stmt (basic_block bb, gimple_st
> ? ? ? ? ? ?fprintf (dump_file, " ?Flagged to clear EH edges.\n");
> ? ? ? ?}
> ? ? }
> -
> - ?/* Queue the statement to be re-scanned after all the
> - ? ? AVAIL_EXPRS have been processed. ?The change buffer stack for
> - ? ? all the pushed statements will be processed when this queue
> - ? ? is emptied. ?*/
> - ?if (may_have_exposed_new_symbols)
> - ? ?VEC_safe_push (gimple, heap, stmts_to_rescan, gsi_stmt (si));
> ?}
>
> ?/* Search for an existing instance of STMT in the AVAIL_EXPRS table.
> Index: testsuite/gcc.dg/pr41454.c
> ===================================================================
> --- testsuite/gcc.dg/pr41454.c ?(revision 0)
> +++ testsuite/gcc.dg/pr41454.c ?(revision 0)
> @@ -0,0 +1,22 @@
> +/* { dg-do run } */
> +/* { dg-options "-O2 -fno-tree-vrp" } */
> +
> +extern void abort (void);
> +
> +int main ()
> +{
> + ?int BM_tab2[0400];
> + ?int *BM_tab = BM_tab2;
> + ?int *BM_tab_base;
> +
> + ?BM_tab_base = BM_tab;
> + ?BM_tab += 0400;
> + ?while (BM_tab_base != BM_tab)
> + ? ?{
> + ? ? ?*--BM_tab = 6;
> + ? ? ?*--BM_tab = 6;
> + ? ?}
> + ?if (BM_tab2[0] != 6)
> + ? ?abort ();
> + ?return 0;
> +}
>