| Message ID | or8vxi9yjm.fsf@livre.localdomain |
|---|---|
| State | New |
| Headers | show |
On Mon, Feb 14, 2011 at 9:00 PM, Alexandre Oliva <aoliva@redhat.com> wrote: > On Feb 14, 2011, Richard Guenther <richard.guenther@gmail.com> wrote: > >> referenced-var-lookup-fn-arg.patch, for-each-referenced-var-fn-arg.patch, >> estimated-stack-frame-size-cgraph-node-arg.patch and >> compute-inline-params-ret-void.patch are obvious (and ok), you can apply >> them independently now. > > Thanks > >> which does look like a hack. Looking at decl_address_invariant_p it >> _seems_ that the test >> || DECL_CONTEXT (op) == current_function_decl >> || decl_function_context (op) == current_function_decl) >> could simply be >> DECL_CONTEXT (op) && TREE_CODE (DECL_CONTEX (op)) == FUNCTION_DECL > > Err... How would this tell whether the decl is local to the current > function? > >> that said, I'm not sure about the necessity of this patch at this stage, >> I'd like to defer it (or rather a better solution) to 4.7 if possible. >> (you might want to open a bug so we don't forget) > > *nod* > >> Does dont-push-cfun-in-estimate-stack-frame-size.patch depend on this >> patch? > > Nope. > >> I guess dont-push-cfun-in-estimate-stack-frame-size.patch depends on >> ipa-inline-stack-growth-referenced-vars-pr47106.patch at least? > > Yup. > >> Similar dont-push-cfun-in-analyze-function.patch? > > Yep, but the analyze_function change also depends on the > current_function_decl setting within the body size estimation. > >> ipa-inline-stack-growth-referenced-vars-pr47106.patch was attached twice, > > Oops, sorry, I meant to attach the -compare patch the second time. > >> so I'm not sure about inter-dependencies. If the do-not-set-cfun patches >> pass testing ontop of the obvious cleanup patches they are ok as well. > > Ok, I'll retest and install. > >> Now onto ipa-inline-stack-growth-referenced-vars-pr47106.patch > >> it looks ok apart from the new pass_inlinable stuff, that at least sounds >> odd and I'd want Honza to have a look here. The testcase you say >> the patch breaks otherwise redefines a _static_ inline function > > Err, no, it's 3 separate functions with different names (foo, __foo and > ____foo) > >> I also wonder why computing -> inlineable should have any effect on >> that testcase ... > > Computing inlinable early enables edges to not be marked as > non-inlinable early. Computing disregard_* early enables cycles to be > broken at the right spots when deciding the order in which we're going > to process functions. See the comment right before the two occurrences > of disregard in ipa.c:cgraph_postorder(). Ah ok, that indeed makes sense. Computing node->local separately from inline-limits is a nice cleanup indeed. Let's wait for Honzas comment and if he's happy with that part the patch is ok. Thanks, Richard. >> So, can we get the approved pre-requesties applied? > > Will do shortly. > > Thanks for the reviews. > > > > > -- > Alexandre Oliva, freedom fighter http://FSFLA.org/~lxoliva/ > You must be the change you wish to see in the world. -- Gandhi > Be Free! -- http://FSFLA.org/ FSF Latin America board member > Free Software Evangelist Red Hat Brazil Compiler Engineer > >
FTR, testing and reporting only, not to be installed. Index: gcc/cfgexpand.c =================================================================== --- gcc/cfgexpand.c.orig 2011-02-10 09:27:16.004373533 -0200 +++ gcc/cfgexpand.c 2011-02-10 09:29:49.418374663 -0200 @@ -1311,18 +1311,41 @@ create_stack_guard (void) crtl->stack_protect_guard = guard; } +/* A subroutine of expand_used_vars. Walk down through the BLOCK tree + expanding variables. Those variables that can be put into registers + are allocated pseudos; those that can't are put on the stack. + + TOPLEVEL is true if this is the outermost BLOCK. */ + +static HOST_WIDE_INT +account_used_vars_for_block (tree block, bool toplevel) +{ + tree t; + HOST_WIDE_INT size = 0; + + /* Expand all variables at this level. */ + for (t = BLOCK_VARS (block); t ; t = DECL_CHAIN (t)) + size += expand_one_var (t, toplevel, false); + + /* Expand all variables at containing levels. */ + for (t = BLOCK_SUBBLOCKS (block); t ; t = BLOCK_CHAIN (t)) + size += account_used_vars_for_block (t, false); + + return size; +} + /* Prepare for expanding variables. */ static void -init_vars_expansion (void) +init_vars_expansion (tree decl) { tree t; unsigned ix; /* Set TREE_USED on all variables in the local_decls. */ - FOR_EACH_LOCAL_DECL (cfun, ix, t) + FOR_EACH_LOCAL_DECL (DECL_STRUCT_FUNCTION (decl), ix, t) TREE_USED (t) = 1; /* Clear TREE_USED on all variables associated with a block scope. */ - clear_tree_used (DECL_INITIAL (current_function_decl)); + clear_tree_used (DECL_INITIAL (decl)); /* Initialize local stack smashing state. */ has_protected_decls = false; @@ -1354,6 +1377,8 @@ HOST_WIDE_INT estimated_stack_frame_size (struct cgraph_node *node) { HOST_WIDE_INT size = 0; + HOST_WIDE_INT delta; + unsigned ix; size_t i; tree var; referenced_var_iterator rvi; @@ -1377,6 +1402,34 @@ estimated_stack_frame_size (struct cgrap fini_vars_expansion (); } + delta = size; + + init_vars_expansion (node->decl); + + FOR_EACH_LOCAL_DECL (fn, ix, var) + { + /* TREE_USED marks local variables that do not appear in lexical + blocks. We don't want to expand those that do twice. */ + if (TREE_USED (var)) + delta -= expand_one_var (var, true, false); + } + delta -= account_used_vars_for_block (DECL_INITIAL (node->decl), true); + + if (stack_vars_num > 0) + { + /* Fake sorting the stack vars for account_stack_vars (). */ + stack_vars_sorted = XNEWVEC (size_t, stack_vars_num); + for (i = 0; i < stack_vars_num; ++i) + stack_vars_sorted[i] = i; + delta -= account_stack_vars (); + fini_vars_expansion (); + } + + if (delta) + inform (input_location, + "guestimated stack size for %q+D changed by %li to %li", + node->decl, (long)delta, (long)size); + current_function_decl = save; pop_cfun (); @@ -1400,7 +1453,7 @@ expand_used_vars (void) frame_phase = off ? align - off : 0; } - init_vars_expansion (); + init_vars_expansion (current_function_decl); for (i = 0; i < SA.map->num_partitions; i++) {