This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: Remove prev_try pointer from ehcleanup region datastructure


On Sat, May 9, 2009 at 1:49 PM, Jan Hubicka <hubicka@ucw.cz> wrote:
> Hi,
> since I added prev_try verification to the verify_eh_tree, I am
> constantly getting new side cases where prev_try ends up being wrong.
> This pointer is used only at once place: in foreach_reachable_handler
> to skip cleanup regions after first region is found (since CFG will have
> RESX edges so dirrect edges are not neccesary) and as found by my
> previous patch it does not serve its purpose well anyway.
>
> It is a lot easier to just walk the tree here (via find_prev_try) and I
> can't imagine this causing noticeable slowdowns. ?I benchmarked build of
> libstdc++ and Ada and found no difference. ?It made sense when EH tree
> was largely static datastructure built once by fronend, but now when
> it is being dynamically updated all the time, it is not doing any good
> job.
>
> Botstrapped/regtested x86_64-linux, OK?

Ok.

Thanks,
Richard.

> Honza
>
> ? ? ? ?* tree-eh.c (struct leh_state): Remove prev_try.
> ? ? ? ?(lower_try_finally, lower_catch, lower_eh_filter, lower_cleanup): Do
> ? ? ? ?not track prev_try.
> ? ? ? ?* except.c (gen_eh_region_cleanup, duplicate_eh_regions,
> ? ? ? ?copy_eh_region_1, copy_eh_region, redirect_eh_edge_to_label,
> ? ? ? ?remove_eh_handler_and_replace, foreach_reachable_handler,
> ? ? ? ?verify_eh_region, verify_eh_tree): Remove tracking of prev_try pointer.
> ? ? ? ?* except.h (struct eh_region): Remove eh_region_u_cleanup.
> ? ? ? ?(gen_eh_region_cleanup): Update prototype.
> Index: tree-eh.c
> ===================================================================
> --- tree-eh.c ? (revision 147317)
> +++ tree-eh.c ? (working copy)
> @@ -352,7 +352,6 @@ struct leh_state
> ? ? ?correspond to variables of the same name in cfun->eh, which we
> ? ? ?don't have easy access to. ?*/
> ? struct eh_region *cur_region;
> - ?struct eh_region *prev_try;
>
> ? /* Processing of TRY_FINALLY requires a bit more state. ?This is
> ? ? ?split out into a separate structure so that we don't have to
> @@ -1566,12 +1565,11 @@ lower_try_finally (struct leh_state *sta
> ? this_tf.outer = state;
> ? if (using_eh_for_cleanups_p)
> ? ? this_tf.region
> - ? ? ?= gen_eh_region_cleanup (state->cur_region, state->prev_try);
> + ? ? ?= gen_eh_region_cleanup (state->cur_region);
> ? else
> ? ? this_tf.region = NULL;
>
> ? this_state.cur_region = this_tf.region;
> - ?this_state.prev_try = state->prev_try;
> ? this_state.tf = &this_tf;
>
> ? lower_eh_constructs_1 (&this_state, gimple_try_eval(tp));
> @@ -1650,7 +1648,6 @@ lower_catch (struct leh_state *state, gi
>
> ? try_region = gen_eh_region_try (state->cur_region);
> ? this_state.cur_region = try_region;
> - ?this_state.prev_try = try_region;
> ? this_state.tf = state->tf;
>
> ? lower_eh_constructs_1 (&this_state, gimple_try_eval (tp));
> @@ -1672,7 +1669,6 @@ lower_catch (struct leh_state *state, gi
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? gimple_catch_types (gcatch));
>
> ? ? ? this_state.cur_region = catch_region;
> - ? ? ?this_state.prev_try = state->prev_try;
> ? ? ? lower_eh_constructs_1 (&this_state, gimple_catch_handler (gcatch));
>
> ? ? ? eh_label = create_artificial_label ();
> @@ -1719,10 +1715,6 @@ lower_eh_filter (struct leh_state *state
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? gimple_eh_filter_types (inner));
> ? this_state = *state;
> ? this_state.cur_region = this_region;
> - ?/* For must not throw regions any cleanup regions inside it
> - ? ? can't reach outer catch regions. ?*/
> - ?if (gimple_eh_filter_must_not_throw (inner))
> - ? ?this_state.prev_try = NULL;
>
> ? lower_eh_constructs_1 (&this_state, gimple_try_eval (tp));
>
> @@ -1759,7 +1751,7 @@ lower_cleanup (struct leh_state *state,
> ? ? ? return result;
> ? ? }
>
> - ?this_region = gen_eh_region_cleanup (state->cur_region, state->prev_try);
> + ?this_region = gen_eh_region_cleanup (state->cur_region);
> ? this_state = *state;
> ? this_state.cur_region = this_region;
>
> Index: except.c
> ===================================================================
> --- except.c ? ?(revision 147317)
> +++ except.c ? ?(working copy)
> @@ -338,10 +338,9 @@ gen_eh_region (enum eh_region_type type,
> ?}
>
> ?struct eh_region *
> -gen_eh_region_cleanup (struct eh_region *outer, struct eh_region *prev_try)
> +gen_eh_region_cleanup (struct eh_region *outer)
> ?{
> ? struct eh_region *cleanup = gen_eh_region (ERT_CLEANUP, outer);
> - ?cleanup->u.cleanup.prev_try = prev_try;
> ? return cleanup;
> ?}
>
> @@ -1183,8 +1182,8 @@ duplicate_eh_regions_1 (eh_region old, e
> ? return ret;
> ?}
>
> -/* Return prev_try pointers catch subregions of R should
> - ? point to. ?*/
> +/* Look for first outer region of R (or R itself) that is
> + ? TRY region. Return NULL if none. ?*/
>
> ?static struct eh_region *
> ?find_prev_try (struct eh_region * r)
> @@ -1208,7 +1207,7 @@ int
> ?duplicate_eh_regions (struct function *ifun, duplicate_eh_regions_map map,
> ? ? ? ? ? ? ? ? ? ? ?void *data, int copy_region, int outer_region)
> ?{
> - ?eh_region cur, prev_try, old_prev_try, outer, *splice;
> + ?eh_region cur, outer, *splice;
> ? int i, min_region, max_region, eh_offset, cfun_last_region_number;
> ? int num_regions;
>
> @@ -1228,14 +1227,12 @@ duplicate_eh_regions (struct function *i
> ? ? ? max_region = 0;
>
> ? ? ? cur = VEC_index (eh_region, ifun->eh->region_array, copy_region);
> - ? ? ?old_prev_try = find_prev_try (cur);
> ? ? ? duplicate_eh_regions_0 (cur, &min_region, &max_region);
> ? ? }
> ? else
> ? ? {
> ? ? ? min_region = 1;
> ? ? ? max_region = ifun->eh->last_region_number;
> - ? ? ?old_prev_try = NULL;
> ? ? }
> ? num_regions = max_region - min_region + 1;
> ? cfun_last_region_number = cfun->eh->last_region_number;
> @@ -1302,12 +1299,6 @@ duplicate_eh_regions (struct function *i
> ? ? if (cur && cur->tree_label)
> ? ? ? cur->tree_label = map (cur->tree_label, data);
>
> - ?/* Search for the containing ERT_TRY region to fix up
> - ? ? the prev_try short-cuts for ERT_CLEANUP regions. ?*/
> - ?prev_try = NULL;
> - ?if (outer_region > 0)
> - ? ?prev_try = find_prev_try (VEC_index (eh_region, cfun->eh->region_array, outer_region));
> -
> ? /* Remap all of the internal catch and cleanup linkages. ?Since we
> ? ? ?duplicate entire subtrees, all of the referenced regions will have
> ? ? ?been copied too. ?And since we renumbered them as a block, a simple
> @@ -1354,13 +1345,6 @@ duplicate_eh_regions (struct function *i
> ? ? ? ? ? ?REMAP (cur->u.eh_catch.prev_catch);
> ? ? ? ? ?break;
>
> - ? ? ? case ERT_CLEANUP:
> - ? ? ? ? if (cur->u.cleanup.prev_try != old_prev_try)
> - ? ? ? ? ? REMAP (cur->u.cleanup.prev_try);
> - ? ? ? ? else
> - ? ? ? ? ? cur->u.cleanup.prev_try = prev_try;
> - ? ? ? ? break;
> -
> ? ? ? ?default:
> ? ? ? ? ?break;
> ? ? ? ?}
> @@ -1394,14 +1378,10 @@ copy_eh_region_1 (struct eh_region *old,
>
> ?/* Return new copy of eh region OLD inside region NEW_OUTER.
>
> - ? Copy whole catch-try chain if neccesary and update cleanup region prev_try
> - ? pointers.
> -
> - ? PREV_TRY_MAP points to outer TRY region if it was copied in trace already. ?*/
> + ? Copy whole catch-try chain if neccesary. ?*/
>
> ?static struct eh_region *
> -copy_eh_region (struct eh_region *old, struct eh_region *new_outer,
> - ? ? ? ? ? ? ? struct eh_region *prev_try_map)
> +copy_eh_region (struct eh_region *old, struct eh_region *new_outer)
> ?{
> ? struct eh_region *r, *n, *old_try, *new_try, *ret = NULL;
> ? VEC(eh_region,heap) *catch_list = NULL;
> @@ -1410,11 +1390,6 @@ copy_eh_region (struct eh_region *old, s
> ? ? {
> ? ? ? gcc_assert (old->type != ERT_TRY);
> ? ? ? r = copy_eh_region_1 (old, new_outer);
> - ? ? ?if (r->type == ERT_CLEANUP)
> - ? ? ? ?{
> - ? ? ? ? gcc_assert (r->u.cleanup.prev_try || !prev_try_map);
> - ? ? ? ? ?r->u.cleanup.prev_try = prev_try_map;
> - ? ? ? }
> ? ? ? return r;
> ? ? }
>
> @@ -1477,7 +1452,7 @@ struct eh_region *
> ?redirect_eh_edge_to_label (edge e, tree new_dest_label, bool is_resx,
> ? ? ? ? ? ? ? ? ? ? ? ? ? bool inlinable_call, int region_number)
> ?{
> - ?struct eh_region *outer, *prev_try_map;
> + ?struct eh_region *outer;
> ? struct eh_region *region;
> ? VEC (eh_region, heap) * trace = NULL;
> ? int i;
> @@ -1539,7 +1514,6 @@ redirect_eh_edge_to_label (edge e, tree
> ? ? ? ?}
> ? ? ? outer = VEC_index (eh_region, trace, start_here)->outer;
> ? ? ? gcc_assert (start_here >= 0);
> - ? ? ?prev_try_map = find_prev_try (outer);
>
> ? ? ? /* And now do the dirty job! ?*/
> ? ? ? for (i = start_here; i >= 0; i--)
> @@ -1548,7 +1522,7 @@ redirect_eh_edge_to_label (edge e, tree
> ? ? ? ? ?gcc_assert (!outer || old->outer != outer->outer);
>
> ? ? ? ? ?/* Copy region and update label. ?*/
> - ? ? ? ? r = copy_eh_region (old, outer, prev_try_map);
> + ? ? ? ? r = copy_eh_region (old, outer);
> ? ? ? ? ?VEC_replace (eh_region, trace, i, r);
> ? ? ? ? ?if (r->tree_label && label_to_block (r->tree_label) == old_bb)
> ? ? ? ? ? ?{
> @@ -1588,14 +1562,11 @@ redirect_eh_edge_to_label (edge e, tree
> ? ? ? ? ? ? ? ? }
> ? ? ? ? ? ? }
>
> - ? ? ? ? /* Cleanup regions points to outer TRY blocks. ?*/
> - ? ? ? ? if (r->type == ERT_TRY)
> - ? ? ? ? ? prev_try_map = r;
> ? ? ? ? ?outer = r;
> ? ? ? ?}
>
> ? ? ? if (is_resx || region->type == ERT_THROW)
> - ? ? ? r = copy_eh_region (region, outer, prev_try_map);
> + ? ? ? r = copy_eh_region (region, outer);
> ? ? }
>
> ? VEC_free (eh_region, heap, trace);
> @@ -2674,32 +2645,6 @@ remove_eh_handler_and_replace (struct eh
>
> ? outer = region->outer;
>
> - ?/* When we are moving the region in EH tree, update prev_try pointers. ?*/
> - ?if ((outer != replace || region->type == ERT_TRY) && region->inner)
> - ? ?{
> - ? ? ?struct eh_region *prev_try = find_prev_try (replace);
> - ? ? ?p = region->inner;
> - ? ? ?while (p != region)
> - ? ? ? {
> - ? ? ? ? if (p->type == ERT_CLEANUP)
> - ? ? ? ? ? p->u.cleanup.prev_try = prev_try;
> - ? ? ? ? ?if (p->type != ERT_TRY
> - ? ? ? ? ? ? && p->type != ERT_MUST_NOT_THROW
> - ? ? ? ? ? ? && (p->type != ERT_ALLOWED_EXCEPTIONS
> - ? ? ? ? ? ? ? ? || p->u.allowed.type_list)
> - ? ? ? ? ? ? && p->inner)
> - ? ? ? ? ? p = p->inner;
> - ? ? ? ? else if (p->next_peer)
> - ? ? ? ? ? p = p->next_peer;
> - ? ? ? ? else
> - ? ? ? ? ? {
> - ? ? ? ? ? ? while (p != region && !p->next_peer)
> - ? ? ? ? ? ? ? p = p->outer;
> - ? ? ? ? ? ? if (p != region)
> - ? ? ? ? ? ? ? p = p->next_peer;
> - ? ? ? ? ? }
> - ? ? ? }
> - ? ?}
> ? /* For the benefit of efficiently handling REG_EH_REGION notes,
> ? ? ?replace this region in the region array with its containing
> ? ? ?region. ?Note that previous region deletions may result in
> @@ -3123,7 +3068,7 @@ foreach_reachable_handler (int region_nu
> ? ? ? if (region->type == ERT_CLEANUP)
> ? ? ? ? {
> ? ? ? ? ?enum reachable_code code = RNL_NOT_CAUGHT;
> - ? ? ? ? region = region->u.cleanup.prev_try;
> + ? ? ? ? region = find_prev_try (region->outer);
> ? ? ? ? ?/* Continue looking for outer TRY region until we find one
> ? ? ? ? ? ? that might cath something. ?*/
> ? ? ? ? ? while (region
> @@ -4431,9 +4376,6 @@ dump_eh_tree (FILE * out, struct functio
> ? ? ? switch (i->type)
> ? ? ? ?{
> ? ? ? ?case ERT_CLEANUP:
> - ? ? ? ? if (i->u.cleanup.prev_try)
> - ? ? ? ? ? fprintf (out, " prev try:%i",
> - ? ? ? ? ? ? ? ? ? ?i->u.cleanup.prev_try->region_number);
> ? ? ? ? ?break;
>
> ? ? ? ?case ERT_TRY:
> @@ -4513,21 +4455,13 @@ debug_eh_tree (struct function *fn)
> ?/* Verify EH region invariants. ?*/
>
> ?static bool
> -verify_eh_region (struct eh_region *region, struct eh_region *prev_try)
> +verify_eh_region (struct eh_region *region)
> ?{
> ? bool found = false;
> ? if (!region)
> ? ? return false;
> ? switch (region->type)
> ? ? {
> - ? ?case ERT_CLEANUP:
> - ? ? ?if (region->u.cleanup.prev_try != prev_try)
> - ? ? ? {
> - ? ? ? ? error ("Wrong prev_try pointer in EH region %i",
> - ? ? ? ? ? ? ? ?region->region_number);
> - ? ? ? ? found = true;
> - ? ? ? }
> - ? ? ?break;
> ? ? case ERT_TRY:
> ? ? ? {
> ? ? ? ?struct eh_region *c, *prev = NULL;
> @@ -4574,6 +4508,7 @@ verify_eh_region (struct eh_region *regi
> ? ? ? ? ?found = true;
> ? ? ? ?}
> ? ? ? break;
> + ? ?case ERT_CLEANUP:
> ? ? case ERT_ALLOWED_EXCEPTIONS:
> ? ? case ERT_MUST_NOT_THROW:
> ? ? case ERT_THROW:
> @@ -4581,14 +4516,8 @@ verify_eh_region (struct eh_region *regi
> ? ? case ERT_UNKNOWN:
> ? ? ? gcc_unreachable ();
> ? ? }
> - ?if (region->type == ERT_TRY)
> - ? ?prev_try = region;
> - ?else if (region->type == ERT_MUST_NOT_THROW
> - ? ? ? ? ?|| (region->type == ERT_ALLOWED_EXCEPTIONS
> - ? ? ? ? ? ? ?&& !region->u.allowed.type_list))
> - ? ?prev_try = NULL;
> ? for (region = region->inner; region; region = region->next_peer)
> - ? ?found |= verify_eh_region (region, prev_try);
> + ? ?found |= verify_eh_region (region);
> ? return found;
> ?}
>
> @@ -4672,7 +4601,7 @@ verify_eh_tree (struct function *fun)
> ? ? ? ? ? ? ? ? ? ?}
> ? ? ? ? ? ? ? ? ?if (!err)
> ? ? ? ? ? ? ? ? ? ?for (i = fun->eh->region_tree; i; i = i->next_peer)
> - ? ? ? ? ? ? ? ? ? ? err |= verify_eh_region (i, NULL);
> + ? ? ? ? ? ? ? ? ? ? err |= verify_eh_region (i);
>
> ? ? ? ? ? ? ? ? ?if (err)
> ? ? ? ? ? ? ? ? ? ?{
> Index: except.h
> ===================================================================
> --- except.h ? ?(revision 147317)
> +++ except.h ? ?(working copy)
> @@ -85,12 +85,6 @@ struct GTY(()) eh_region
> ? ? struct eh_region_u_throw {
> ? ? ? tree type;
> ? ? } GTY ((tag ("ERT_THROW"))) eh_throw;
> -
> - ? ?/* Retain the cleanup expression even after expansion so that
> - ? ? ? we can match up fixup regions. ?*/
> - ? ?struct eh_region_u_cleanup {
> - ? ? ?struct eh_region *prev_try;
> - ? ?} GTY ((tag ("ERT_CLEANUP"))) cleanup;
> ? } GTY ((desc ("%0.type"))) u;
>
> ? /* Entry point for this region's handler before landing pads are built. ?*/
> @@ -185,8 +179,7 @@ extern int duplicate_eh_regions (struct
> ?extern void sjlj_emit_function_exit_after (rtx);
> ?extern void default_init_unwind_resume_libfunc (void);
>
> -extern struct eh_region *gen_eh_region_cleanup (struct eh_region *,
> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? struct eh_region *);
> +extern struct eh_region *gen_eh_region_cleanup (struct eh_region *);
> ?extern struct eh_region *gen_eh_region_try (struct eh_region *);
> ?extern struct eh_region *gen_eh_region_catch (struct eh_region *, tree);
> ?extern struct eh_region *gen_eh_region_allowed (struct eh_region *, tree);
>


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]