On Mon, Jan 23, 2012 at 6:01 PM, Michael Matz <m...@suse.de> wrote: > Hi, > > On Sat, 21 Jan 2012, Eric Botcazou wrote: > >> > Trivially fixing the thinko (iterating over (work bit-and >> > old_conflict) in the first inner loop) would fix the testcase but in >> > general create too few conflicts, i.e. generate wrong code. I need >> > some time to think about this again. >> >> OK, thanks in advance. > > Sigh. I can't come up with a way to generally speed up the conflict > generation without sometimes adding artificial conflicts. I.e. I'll have > to revert r183305. I can still fix the specific situation of PR46590 by > making sure clobbers are added for all aggregates, not only the (at > gimplification time) address takens. > > So, this is currently in regstrapping, x86_64-linux, all langs+Ada. Okay > for trunk (well, the revert is obvious, but the gimplify.c hunk)? > > > Ciao, > Michael. > > PR tree-optimization/46590 > * cfgexpand.c: Revert last change (r183305). > * gimplify.c (gimplify_bind_expr): Add clobbers for all non-gimple > regs. > > Index: gimplify.c > =================================================================== > --- gimplify.c (revision 183303) > +++ gimplify.c (working copy) > @@ -1226,12 +1226,11 @@ gimplify_bind_expr (tree *expr_p, gimple > if (TREE_CODE (t) == VAR_DECL > && !is_global_var (t) > && DECL_CONTEXT (t) == current_function_decl > - && !DECL_HARD_REGISTER (t) > - && !TREE_THIS_VOLATILE (t) > && !DECL_HAS_VALUE_EXPR_P (t) > /* Only care for variables that have to be in memory. Others > will be rewritten into SSA names, hence moved to the top-level. > */ > - && needs_to_live_in_memory (t)) > + && !is_gimple_reg (t)) > +
Ok with the excessive vertical space removed. Thanks, Richard. > { > tree clobber = build_constructor (TREE_TYPE (t), NULL); > TREE_THIS_VOLATILE (clobber) = 1; > Index: cfgexpand.c > =================================================================== > --- cfgexpand.c (revision 183305) > +++ cfgexpand.c (working copy) > @@ -441,12 +441,11 @@ visit_conflict (gimple stmt ATTRIBUTE_UN > > /* Helper routine for add_scope_conflicts, calculating the active partitions > at the end of BB, leaving the result in WORK. We're called to generate > - conflicts when OLD_CONFLICTS is non-null, otherwise we're just tracking > - liveness. If we generate conflicts then OLD_CONFLICTS stores the bits > - for which we generated conflicts already. */ > + conflicts when FOR_CONFLICT is true, otherwise we're just tracking > + liveness. */ > > static void > -add_scope_conflicts_1 (basic_block bb, bitmap work, bitmap old_conflicts) > +add_scope_conflicts_1 (basic_block bb, bitmap work, bool for_conflict) > { > edge e; > edge_iterator ei; > @@ -483,7 +482,7 @@ add_scope_conflicts_1 (basic_block bb, b > } > else if (!is_gimple_debug (stmt)) > { > - if (old_conflicts > + if (for_conflict > && visit == visit_op) > { > /* If this is the first real instruction in this BB we need > @@ -491,27 +490,16 @@ add_scope_conflicts_1 (basic_block bb, b > Unlike classical liveness for named objects we can't > rely on seeing a def/use of the names we're interested in. > There might merely be indirect loads/stores. We'd not add any > - conflicts for such partitions. We know that we generated > - conflicts between all partitions in old_conflicts already, > - so we need to generate only the new ones, avoiding to > - repeatedly pay the O(N^2) cost for each basic block. */ > + conflicts for such partitions. */ > bitmap_iterator bi; > unsigned i; > - > - EXECUTE_IF_AND_COMPL_IN_BITMAP (work, old_conflicts, 0, i, bi) > + EXECUTE_IF_SET_IN_BITMAP (work, 0, i, bi) > { > unsigned j; > bitmap_iterator bj; > - /* First the conflicts between new and old_conflicts. */ > - EXECUTE_IF_SET_IN_BITMAP (old_conflicts, 0, j, bj) > - add_stack_var_conflict (i, j); > - /* Then the conflicts between only the new members. */ > - EXECUTE_IF_AND_COMPL_IN_BITMAP (work, old_conflicts, i + 1, > - j, bj) > + EXECUTE_IF_SET_IN_BITMAP (work, i + 1, j, bj) > add_stack_var_conflict (i, j); > } > - /* And remember for the next basic block. */ > - bitmap_ior_into (old_conflicts, work); > visit = visit_conflict; > } > walk_stmt_load_store_addr_ops (stmt, work, visit, visit, visit); > @@ -528,7 +516,6 @@ add_scope_conflicts (void) > basic_block bb; > bool changed; > bitmap work = BITMAP_ALLOC (NULL); > - bitmap old_conflicts; > > /* We approximate the live range of a stack variable by taking the first > mention of its name as starting point(s), and by the end-of-scope > @@ -550,18 +537,15 @@ add_scope_conflicts (void) > FOR_EACH_BB (bb) > { > bitmap active = (bitmap)bb->aux; > - add_scope_conflicts_1 (bb, work, NULL); > + add_scope_conflicts_1 (bb, work, false); > if (bitmap_ior_into (active, work)) > changed = true; > } > } > > - old_conflicts = BITMAP_ALLOC (NULL); > - > FOR_EACH_BB (bb) > - add_scope_conflicts_1 (bb, work, old_conflicts); > + add_scope_conflicts_1 (bb, work, true); > > - BITMAP_FREE (old_conflicts); > BITMAP_FREE (work); > FOR_ALL_BB (bb) > BITMAP_FREE (bb->aux);