[PATCH] Fix var-tracking cselib VALUE chains handling (PR debug/43176)
Richard Guenther
rguenther@suse.de
Mon Mar 8 14:25:00 GMT 2010
On Mon, 8 Mar 2010, Jakub Jelinek wrote:
> On Sun, Mar 07, 2010 at 04:25:32PM +0100, Richard Guenther wrote:
> > > Bootstrapped/regtested on x86_64-linux and i686-linux
> > > (--enable-checking=yes,rtl, also --enable-checking=yes and
> > > --enable-checking=yes,rtl with a hack to disable -fvar-tracking-assignments
> > > by default). A backport of this patch (identical except trainling
> > > whitespace changes) to redhat/gcc-4_4-branch has been bootstrapped/regtested
> > > with --enable-checking=yes,rtl on {x86_64,i686,ppc,ppc64,s390,s390x}-linux.
> > >
> > > Ok for trunk?
> >
> > Ok.
>
> On s390-linux unfortunately the patch actually showed one regression:
> FAIL: gcc.c-torture/execute/920501-8.c compilation, -O3 -g (internal compiler error)
> UNRESOLVED: gcc.c-torture/execute/920501-8.c execution, -O3 -g
> (sorry for missing it, I initially assumed it was because this 4.4-RH
> bootstrap/regtest was --enable-checking=yes,rtl while the one I was
> comparing to has been --enable-checking=release).
>
> the problem is that when some VALUE is not in the current dataflow_set and
> some decl (or VALUE) uses it, cselib_*expand_value_rtx* goes through its
> cselib maintained list of locs (which, during
> vt_find_locations/vt_emit_notes never contains any REGs or MEMs, which have
> been invalidated, but just equivalences that are always true through the
> whole function (either constant values or expressions using other VALUEs and
> optionally constants). Var-tracking records these cselib locs VALUE
> dependencies through {add,remove}_cselib_value_chains, but unfortunately
> maintains it only for VALUEs live in the current dataflow_set. This means
> on 920501-8.c that we fail to notice a value chain. E.g. some
> variable (say var) has location
> (mem (value N))
> and (value N) is not in the current dataflow set, but cselib has record that
> (value N) is (plus (value M) (const_int 16)) and (value M) is in the
> dataflow_set and gets marked as changed. We have value_chain links from
> (value M) to (value N), but as (value N) is not in the set, we don't
> have value_chain links from (value N) back to var (which is again in
> dataflow_set).
>
> The following patch fixes it by keeping cselib value_chains around
> permanently from vt_emit_notes start till the end of it (which can save some
> CPU time as we don't need to constantly add and remove those chains, on the
> other side needs perhaps slightly more memory (this is after freeing all
> out hash tables from vt_find_locations though, so it shouldn't affect peak GCC
> memory usages)). When check_changed_vars_{1,2} sees a VALUE that is not in
> the dataflow_set, it recurses through its chains. During x86_64 and
> i686-linux bootstraps together this recursion (i.e. VALUE seen in
> value_chains and not in dataflow_set) happens in roughly 15% of times
> and didn't measurably affect bootstrap/regtest times.
>
> changed_values_stack isn't pretty, but without it trunk on i686 would spend
> hours compiling libiberty/sha1.c - check_changed_vars_0 was called many
> times for the same VALUE and in each case walking its value_chains.
> With it each VALUE is walked at most once.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux trunk
> (--enable-checking=yes,rtl and --enable-checking=release) and 4.4-RH
> ({x86_64,i686,ppc,ppc64,s390,s390x}-linux --enable-checking=yes,rtl).
>
> Ok for trunk?
Ok.
Thanks,
Richard.
> 2010-03-08 Jakub Jelinek <jakub@redhat.com>
>
> * var-tracking.c (remove_cselib_value_chains): Define only for
> ENABLE_CHECKING.
> (dataflow_set_preserve_mem_locs, dataflow_set_remove_mem_locs,
> delete_slot_part, emit_notes_for_differences_1): Don't call
> remove_cselib_value_chains here.
> (set_slot_part, emit_notes_for_differences_2): Don't call
> add_cselib_value_chains here.
> (preserved_values): New vector.
> (preserve_value): New function.
> (add_uses, add_stores, vt_add_function_parameters): Use it
> instead of cselib_preserve_value.
> (changed_values_stack): New vector.
> (check_changed_vars_0): New function.
> (check_changed_vars_1, check_changed_vars_2): Use it.
> (emit_notes_for_changes): Call set_dv_changed (*, false) on all
> changed_values_stack VALUEs.
> (vt_emit_notes): For all preserved_values call
> add_cselib_value_chains. If ENABLE_CHECKING call
> remove_cselib_value_chains before verifying value_chains is empty.
> Initialize and free changed_values_stack.
> (vt_initialize): Initialize preserved_values.
> (vt_finalize): Free preserved_values.
>
> --- gcc/var-tracking.c.jj 2010-03-07 16:45:45.000000000 +0100
> +++ gcc/var-tracking.c 2010-03-08 06:13:15.000000000 +0100
> @@ -2674,6 +2674,7 @@ remove_value_chains (decl_or_value dv, r
> for_each_rtx (&loc, remove_value_chain, dv_as_opaque (dv));
> }
>
> +#if ENABLE_CHECKING
> /* If CSELIB_VAL_PTR of value DV refer to VALUEs, remove backlinks from those
> VALUEs to DV. */
>
> @@ -2686,7 +2687,6 @@ remove_cselib_value_chains (decl_or_valu
> for_each_rtx (&l->loc, remove_value_chain, dv_as_opaque (dv));
> }
>
> -#if ENABLE_CHECKING
> /* Check the order of entries in one-part variables. */
>
> static int
> @@ -3825,8 +3825,6 @@ dataflow_set_preserve_mem_locs (void **s
> if (!var->var_part[0].loc_chain)
> {
> var->n_var_parts--;
> - if (emit_notes && dv_is_value_p (var->dv))
> - remove_cselib_value_chains (var->dv);
> changed = true;
> }
> if (changed)
> @@ -3895,8 +3893,6 @@ dataflow_set_remove_mem_locs (void **slo
> if (!var->var_part[0].loc_chain)
> {
> var->n_var_parts--;
> - if (emit_notes && dv_is_value_p (var->dv))
> - remove_cselib_value_chains (var->dv);
> changed = true;
> }
> if (changed)
> @@ -4654,6 +4650,20 @@ count_with_sets (rtx insn, struct cselib
> #define VAL_EXPR_HAS_REVERSE(x) \
> (RTL_FLAG_CHECK1 ("VAL_EXPR_HAS_REVERSE", (x), CONCAT)->return_val)
>
> +/* All preserved VALUEs. */
> +static VEC (rtx, heap) *preserved_values;
> +
> +/* Ensure VAL is preserved and remember it in a vector for vt_emit_notes.
> + This must be only called when cselib_preserve_only_values will be called
> + with true, the counting phase must use cselib_preserve_value. */
> +
> +static void
> +preserve_value (cselib_val *val)
> +{
> + cselib_preserve_value (val);
> + VEC_safe_push (rtx, heap, preserved_values, val->val_rtx);
> +}
> +
> /* Add uses (register and memory references) LOC which will be tracked
> to VTI (bb)->mos. INSN is instruction which the LOC is part of. */
>
> @@ -4697,7 +4707,7 @@ add_uses (rtx *ploc, void *data)
> mon->type = mo->type;
> mon->u.loc = mo->u.loc;
> mon->insn = mo->insn;
> - cselib_preserve_value (val);
> + preserve_value (val);
> mo->type = MO_VAL_USE;
> mloc = cselib_subst_to_values (XEXP (mloc, 0));
> mo->u.loc = gen_rtx_CONCAT (address_mode,
> @@ -4733,7 +4743,7 @@ add_uses (rtx *ploc, void *data)
> && !cselib_preserved_value_p (val))
> {
> VAL_NEEDS_RESOLUTION (oloc) = 1;
> - cselib_preserve_value (val);
> + preserve_value (val);
> }
> }
> else if (!VAR_LOC_UNKNOWN_P (vloc))
> @@ -4768,7 +4778,7 @@ add_uses (rtx *ploc, void *data)
> mon->type = mo->type;
> mon->u.loc = mo->u.loc;
> mon->insn = mo->insn;
> - cselib_preserve_value (val);
> + preserve_value (val);
> mo->type = MO_VAL_USE;
> mloc = cselib_subst_to_values (XEXP (mloc, 0));
> mo->u.loc = gen_rtx_CONCAT (address_mode,
> @@ -4817,7 +4827,7 @@ add_uses (rtx *ploc, void *data)
> if (!cselib_preserved_value_p (val))
> {
> VAL_NEEDS_RESOLUTION (mo->u.loc) = 1;
> - cselib_preserve_value (val);
> + preserve_value (val);
> }
> }
> else
> @@ -5000,7 +5010,7 @@ add_stores (rtx loc, const_rtx expr, voi
>
> if (val && !cselib_preserved_value_p (val))
> {
> - cselib_preserve_value (val);
> + preserve_value (val);
> mo->type = MO_VAL_USE;
> mloc = cselib_subst_to_values (XEXP (mloc, 0));
> mo->u.loc = gen_rtx_CONCAT (address_mode, val->val_rtx, mloc);
> @@ -5073,7 +5083,7 @@ add_stores (rtx loc, const_rtx expr, voi
> {
> micro_operation *nmo = VTI (bb)->mos + VTI (bb)->n_mos++;
>
> - cselib_preserve_value (oval);
> + preserve_value (oval);
>
> nmo->type = MO_VAL_USE;
> nmo->u.loc = gen_rtx_CONCAT (mode, oval->val_rtx, oloc);
> @@ -5161,7 +5171,7 @@ add_stores (rtx loc, const_rtx expr, voi
> if (preserve)
> {
> VAL_NEEDS_RESOLUTION (loc) = resolve;
> - cselib_preserve_value (v);
> + preserve_value (v);
> }
> if (mo->type == MO_CLOBBER)
> VAL_EXPR_IS_CLOBBERED (loc) = 1;
> @@ -6094,8 +6104,6 @@ set_slot_part (dataflow_set *set, rtx lo
> *slot = var;
> pos = 0;
> nextp = &var->var_part[0].loc_chain;
> - if (emit_notes && dv_is_value_p (dv))
> - add_cselib_value_chains (dv);
> }
> else if (onepart)
> {
> @@ -6483,11 +6491,7 @@ delete_slot_part (dataflow_set *set, rtx
> changed = true;
> var->n_var_parts--;
> if (emit_notes)
> - {
> - var->cur_loc_changed = true;
> - if (var->n_var_parts == 0 && dv_is_value_p (var->dv))
> - remove_cselib_value_chains (var->dv);
> - }
> + var->cur_loc_changed = true;
> while (pos < var->n_var_parts)
> {
> var->var_part[pos] = var->var_part[pos + 1];
> @@ -6968,6 +6972,42 @@ DEF_VEC_ALLOC_P (variable, heap);
>
> static VEC (variable, heap) *changed_variables_stack;
>
> +/* VALUEs with no variables that need set_dv_changed (val, false)
> + called before check_changed_vars_3. */
> +
> +static VEC (rtx, heap) *changed_values_stack;
> +
> +/* Helper function for check_changed_vars_1 and check_changed_vars_2. */
> +
> +static void
> +check_changed_vars_0 (decl_or_value dv, htab_t htab)
> +{
> + value_chain vc
> + = (value_chain) htab_find_with_hash (value_chains, dv, dv_htab_hash (dv));
> +
> + if (vc == NULL)
> + return;
> + for (vc = vc->next; vc; vc = vc->next)
> + if (!dv_changed_p (vc->dv))
> + {
> + variable vcvar
> + = (variable) htab_find_with_hash (htab, vc->dv,
> + dv_htab_hash (vc->dv));
> + if (vcvar)
> + {
> + set_dv_changed (vc->dv, true);
> + VEC_safe_push (variable, heap, changed_variables_stack, vcvar);
> + }
> + else if (dv_is_value_p (vc->dv))
> + {
> + set_dv_changed (vc->dv, true);
> + VEC_safe_push (rtx, heap, changed_values_stack,
> + dv_as_value (vc->dv));
> + check_changed_vars_0 (vc->dv, htab);
> + }
> + }
> +}
> +
> /* Populate changed_variables_stack with variable_def pointers
> that need variable_was_changed called on them. */
>
> @@ -6979,24 +7019,7 @@ check_changed_vars_1 (void **slot, void
>
> if (dv_is_value_p (var->dv)
> || TREE_CODE (dv_as_decl (var->dv)) == DEBUG_EXPR_DECL)
> - {
> - value_chain vc
> - = (value_chain) htab_find_with_hash (value_chains, var->dv,
> - dv_htab_hash (var->dv));
> -
> - if (vc == NULL)
> - return 1;
> - for (vc = vc->next; vc; vc = vc->next)
> - if (!dv_changed_p (vc->dv))
> - {
> - variable vcvar
> - = (variable) htab_find_with_hash (htab, vc->dv,
> - dv_htab_hash (vc->dv));
> - if (vcvar)
> - VEC_safe_push (variable, heap, changed_variables_stack,
> - vcvar);
> - }
> - }
> + check_changed_vars_0 (var->dv, htab);
> return 1;
> }
>
> @@ -7010,23 +7033,7 @@ check_changed_vars_2 (variable var, htab
> variable_was_changed (var, NULL);
> if (dv_is_value_p (var->dv)
> || TREE_CODE (dv_as_decl (var->dv)) == DEBUG_EXPR_DECL)
> - {
> - value_chain vc
> - = (value_chain) htab_find_with_hash (value_chains, var->dv,
> - dv_htab_hash (var->dv));
> -
> - if (vc == NULL)
> - return;
> - for (vc = vc->next; vc; vc = vc->next)
> - if (!dv_changed_p (vc->dv))
> - {
> - variable vcvar
> - = (variable) htab_find_with_hash (htab, vc->dv,
> - dv_htab_hash (vc->dv));
> - if (vcvar)
> - check_changed_vars_2 (vcvar, htab);
> - }
> - }
> + check_changed_vars_0 (var->dv, htab);
> }
>
> /* For each changed decl (except DEBUG_EXPR_DECLs) recompute
> @@ -7094,6 +7101,9 @@ emit_notes_for_changes (rtx insn, enum e
> while (VEC_length (variable, changed_variables_stack) > 0)
> check_changed_vars_2 (VEC_pop (variable, changed_variables_stack),
> htab);
> + while (VEC_length (rtx, changed_values_stack) > 0)
> + set_dv_changed (dv_from_value (VEC_pop (rtx, changed_values_stack)),
> + false);
> htab_traverse (changed_variables, check_changed_vars_3, htab);
> }
>
> @@ -7135,8 +7145,6 @@ emit_notes_for_differences_1 (void **slo
> gcc_assert (old_var->n_var_parts == 1);
> for (lc = old_var->var_part[0].loc_chain; lc; lc = lc->next)
> remove_value_chains (old_var->dv, lc->loc);
> - if (dv_is_value_p (old_var->dv))
> - remove_cselib_value_chains (old_var->dv);
> }
> variable_was_changed (empty_var, NULL);
> /* Continue traversing the hash table. */
> @@ -7222,8 +7230,6 @@ emit_notes_for_differences_2 (void **slo
> gcc_assert (new_var->n_var_parts == 1);
> for (lc = new_var->var_part[0].loc_chain; lc; lc = lc->next)
> add_value_chains (new_var->dv, lc->loc);
> - if (dv_is_value_p (new_var->dv))
> - add_cselib_value_chains (new_var->dv);
> }
> for (i = 0; i < new_var->n_var_parts; i++)
> new_var->var_part[i].cur_loc = NULL;
> @@ -7546,7 +7552,15 @@ vt_emit_notes (void)
> emit_notes = true;
>
> if (MAY_HAVE_DEBUG_INSNS)
> - changed_variables_stack = VEC_alloc (variable, heap, 40);
> + {
> + unsigned int i;
> + rtx val;
> +
> + for (i = 0; VEC_iterate (rtx, preserved_values, i, val); i++)
> + add_cselib_value_chains (dv_from_value (val));
> + changed_variables_stack = VEC_alloc (variable, heap, 40);
> + changed_values_stack = VEC_alloc (rtx, heap, 40);
> + }
>
> dataflow_set_init (&cur);
>
> @@ -7568,12 +7582,22 @@ vt_emit_notes (void)
> emit_notes_for_differences_1,
> shared_hash_htab (empty_shared_hash));
> if (MAY_HAVE_DEBUG_INSNS)
> - gcc_assert (htab_elements (value_chains) == 0);
> + {
> + unsigned int i;
> + rtx val;
> +
> + for (i = 0; VEC_iterate (rtx, preserved_values, i, val); i++)
> + remove_cselib_value_chains (dv_from_value (val));
> + gcc_assert (htab_elements (value_chains) == 0);
> + }
> #endif
> dataflow_set_destroy (&cur);
>
> if (MAY_HAVE_DEBUG_INSNS)
> - VEC_free (variable, heap, changed_variables_stack);
> + {
> + VEC_free (variable, heap, changed_variables_stack);
> + VEC_free (rtx, heap, changed_values_stack);
> + }
>
> #ifdef ENABLE_RTL_CHECKING
> pointer_map_destroy (emitted_notes);
> @@ -7699,7 +7723,7 @@ vt_add_function_parameters (void)
> cselib. */
> if (val)
> {
> - cselib_preserve_value (val);
> + preserve_value (val);
> set_variable_part (out, val->val_rtx, dv, offset,
> VAR_INIT_STATUS_INITIALIZED, NULL, INSERT);
> dv = dv_from_value (val->val_rtx);
> @@ -7747,6 +7771,7 @@ vt_initialize (void)
> scratch_regs = BITMAP_ALLOC (NULL);
> valvar_pool = create_alloc_pool ("small variable_def pool",
> sizeof (struct variable_def), 256);
> + preserved_values = VEC_alloc (rtx, heap, 256);
> }
> else
> {
> @@ -8023,6 +8048,7 @@ vt_finalize (void)
> htab_delete (value_chains);
> free_alloc_pool (value_chain_pool);
> free_alloc_pool (valvar_pool);
> + VEC_free (rtx, heap, preserved_values);
> cselib_finish ();
> BITMAP_FREE (scratch_regs);
> scratch_regs = NULL;
>
> Jakub
>
>
--
Richard Guenther <rguenther@suse.de>
Novell / SUSE Labs
SUSE LINUX Products GmbH - Nuernberg - AG Nuernberg - HRB 16746 - GF: Markus Rex
More information about the Gcc-patches
mailing list