On Thu, Sep 19, 2013 at 2:48 PM, Jeff Law <l...@redhat.com> wrote:
>
> I find it amazing to look at code I wrote in the past, the occasional WTF
> always makes it worth it.
>
> The code to manage the temporary expression & const/copies tables in
> tree-ssa-dom.c around jump threading looks overly convoluted. In particular
> the code reused an existing unwind point for the temporary expression stack
> when threading one outgoing edge of a block.
>
> I can only guess this was in response to that code at one time being much
> more expensive than it is now. Given how cheap this code is now, handling
> the temporary expression stack in the most obvious way seems much wiser.
>
> The refactoring should also make it easier to get some missing expressions
> into the table without tons of unwanted code duplication.
>
> Bootstrapped and regression tested on x86_64-unknown-linux-gnu. Installed on
> the trunk.
>
>
>
> commit f71b6fa579d113c2de9ec0ab921bbd2dcc7be43c
> Author: Jeff Law <l...@redhat.com>
> Date: Thu Sep 19 05:54:23 2013 -0600
>
> * tree-ssa-dom.c (record_temporary_equivalences): New function
> split out of dom_opt_dom_walker::after_dom_children.
> (dom_opt_dom_walker::thread_across_edge): Move common code
> in here from dom_opt_dom_walker::after_dom_children.
> (dom_opt_dom_walker::after_dom_children): Corresponding
> simplifictions.
>
> diff --git a/gcc/ChangeLog b/gcc/ChangeLog
> index be5b1d9..6c5f5d6 100644
> --- a/gcc/ChangeLog
> +++ b/gcc/ChangeLog
> @@ -1,3 +1,11 @@
> +2013-09-17 Jeff Law <l...@redhat.com>
> +
> + * tree-ssa-dom.c (record_temporary_equivalences): New function
> + split out of dom_opt_dom_walker::after_dom_children.
> + (dom_opt_dom_walker::thread_across_edge): Move common code
> + in here from dom_opt_dom_walker::after_dom_children.
> + (dom_opt_dom_walker::after_dom_children): Corresponding
> simplifictions.
> +
> 2013-09-19 Jan Hubicka <j...@suse.cz>
>
> * cgraph.c (cgraph_create_edge_1): Avoid uninitialized read
> diff --git a/gcc/tree-ssa-dom.c b/gcc/tree-ssa-dom.c
> index aac7aa4..f561386 100644
> --- a/gcc/tree-ssa-dom.c
> +++ b/gcc/tree-ssa-dom.c
> @@ -1070,6 +1070,31 @@ simplify_stmt_for_jump_threading (gimple stmt,
> return lookup_avail_expr (stmt, false);
> }
>
> +static void
> +record_temporary_equivalences (edge e)
> +{
This new function lacks a comment.
Richard.
> + int i;
> + struct edge_info *edge_info = (struct edge_info *) e->aux;
> +
> + /* If we have info associated with this edge, record it into
> + our equivalence tables. */
> + if (edge_info)
> + {
> + cond_equivalence *eq;
> + tree lhs = edge_info->lhs;
> + tree rhs = edge_info->rhs;
> +
> + /* If we have a simple NAME = VALUE equivalence, record it. */
> + if (lhs && TREE_CODE (lhs) == SSA_NAME)
> + record_const_or_copy (lhs, rhs);
> +
> + /* If we have 0 = COND or 1 = COND equivalences, record them
> + into our expression hash tables. */
> + for (i = 0; edge_info->cond_equivalences.iterate (i, &eq); ++i)
> + record_cond (eq);
> + }
> +}
> +
> /* Wrapper for common code to attempt to thread an edge. For example,
> it handles lazily building the dummy condition and the bookkeeping
> when jump threading is successful. */
> @@ -1083,9 +1108,27 @@ dom_opt_dom_walker::thread_across_edge (edge e)
> integer_zero_node, integer_zero_node,
> NULL, NULL);
>
> + /* Push a marker on both stacks so we can unwind the tables back to their
> + current state. */
> + avail_exprs_stack.safe_push (NULL);
> + const_and_copies_stack.safe_push (NULL_TREE);
> +
> + /* Traversing E may result in equivalences we can utilize. */
> + record_temporary_equivalences (e);
> +
> + /* With all the edge equivalences in the tables, go ahead and attempt
> + to thread through E->dest. */
> ::thread_across_edge (dummy_cond_, e, false,
> &const_and_copies_stack,
> simplify_stmt_for_jump_threading);
> +
> + /* And restore the various tables to their state before
> + we threaded this edge.
> +
> + XXX The code in tree-ssa-threadedge.c will restore the state of
> + the const_and_copies table. We we just have to restore the expression
> + table. */
> + remove_local_expressions_from_table ();
> }
>
> /* PHI nodes can create equivalences too.
> @@ -1905,9 +1948,6 @@ dom_opt_dom_walker::after_dom_children (basic_block
> bb)
> && (single_succ_edge (bb)->flags & EDGE_ABNORMAL) == 0
> && potentially_threadable_block (single_succ (bb)))
> {
> - /* Push a marker on the stack, which thread_across_edge expects
> - and will remove. */
> - const_and_copies_stack.safe_push (NULL_TREE);
> thread_across_edge (single_succ_edge (bb));
> }
> else if ((last = last_stmt (bb))
> @@ -1923,79 +1963,15 @@ dom_opt_dom_walker::after_dom_children (basic_block
> bb)
> /* Only try to thread the edge if it reaches a target block with
> more than one predecessor and more than one successor. */
> if (potentially_threadable_block (true_edge->dest))
> - {
> - struct edge_info *edge_info;
> - unsigned int i;
> -
> - /* Push a marker onto the available expression stack so that we
> - unwind any expressions related to the TRUE arm before
> processing
> - the false arm below. */
> - avail_exprs_stack.safe_push (NULL);
> - const_and_copies_stack.safe_push (NULL_TREE);
> -
> - edge_info = (struct edge_info *) true_edge->aux;
> -
> - /* If we have info associated with this edge, record it into
> - our equivalence tables. */
> - if (edge_info)
> - {
> - cond_equivalence *eq;
> - tree lhs = edge_info->lhs;
> - tree rhs = edge_info->rhs;
> -
> - /* If we have a simple NAME = VALUE equivalence, record it.
> */
> - if (lhs && TREE_CODE (lhs) == SSA_NAME)
> - record_const_or_copy (lhs, rhs);
> -
> - /* If we have 0 = COND or 1 = COND equivalences, record them
> - into our expression hash tables. */
> - for (i = 0; edge_info->cond_equivalences.iterate (i, &eq);
> ++i)
> - record_cond (eq);
> - }
> -
> - thread_across_edge (true_edge);
> -
> - /* And restore the various tables to their state before
> - we threaded this edge. */
> - remove_local_expressions_from_table ();
> - }
> + thread_across_edge (true_edge);
>
> /* Similarly for the ELSE arm. */
> if (potentially_threadable_block (false_edge->dest))
> - {
> - struct edge_info *edge_info;
> - unsigned int i;
> -
> - const_and_copies_stack.safe_push (NULL_TREE);
> - edge_info = (struct edge_info *) false_edge->aux;
> -
> - /* If we have info associated with this edge, record it into
> - our equivalence tables. */
> - if (edge_info)
> - {
> - cond_equivalence *eq;
> - tree lhs = edge_info->lhs;
> - tree rhs = edge_info->rhs;
> -
> - /* If we have a simple NAME = VALUE equivalence, record it.
> */
> - if (lhs && TREE_CODE (lhs) == SSA_NAME)
> - record_const_or_copy (lhs, rhs);
> -
> - /* If we have 0 = COND or 1 = COND equivalences, record them
> - into our expression hash tables. */
> - for (i = 0; edge_info->cond_equivalences.iterate (i, &eq);
> ++i)
> - record_cond (eq);
> - }
> + thread_across_edge (false_edge);
>
> - /* Now thread the edge. */
> - thread_across_edge (false_edge);
> -
> - /* No need to remove local expressions from our tables
> - or restore vars to their original value as that will
> - be done immediately below. */
> - }
> }
>
> + /* These remove expressions local to BB from the tables. */
> remove_local_expressions_from_table ();
> restore_vars_to_original_value ();
> }
>
