On Mon, Dec 14, 2015 at 9:44 AM, Jan Beulich <jbeul...@suse.com> wrote: >>>> On 14.12.15 at 09:35, <richard.guent...@gmail.com> wrote: >> On Fri, Dec 11, 2015 at 2:54 PM, Bernd Schmidt <bschm...@redhat.com> wrote: >>> On 12/11/2015 02:48 PM, Jan Beulich wrote: >>>> >>>> Function (or more narrow) scope static variables (as well as others not >>>> placed on the stack) should also not have any effect on the stack >>>> alignment. I noticed the issue first with Linux'es dynamic_pr_debug() >>>> construct using an 8-byte aligned sub-file-scope local variable. >>>> >>>> According to my checking bad behavior started with 4.6.x (4.5.3 was >>>> still okay), but generated code got quite a bit worse as of 4.9.0. >>>> >>>> [v4: Bail early, using is_global_var(), as requested by Bernd.] >>> >>> >>> In case I haven't made it obvious, this is OK. >> >> But I wonder if it makes sense because shortly after the early-out we check >> >> if (TREE_STATIC (var) >> || DECL_EXTERNAL (var) >> || (TREE_CODE (origvar) == SSA_NAME && use_register_for_decl >> (var))) >> >> so either there are obvious cleanup opportunities left or the patch is >> broken. > > Looks like a cleanup opportunity I overlooked when following > Bernd's advice.
Well, looking at callers it doesn't sound so obvious ... /* Expand all variables used in the function. */ static rtx_insn * expand_used_vars (void) { .. len = vec_safe_length (cfun->local_decls); FOR_EACH_LOCAL_DECL (cfun, i, var) { ... /* We didn't set a block for static or extern because it's hard to tell the difference between a global variable (re)declared in a local scope, and one that's really declared there to begin with. And it doesn't really matter much, since we're not giving them stack space. Expand them now. */ else if (TREE_STATIC (var) || DECL_EXTERNAL (var)) expand_now = true; ... if (expand_now) expand_one_var (var, true, true); but then you'll immediately return. So to make sense of your patch a larger refactorig is necessary. expand_one_var also later contains else if (DECL_EXTERNAL (var)) ; else if (DECL_HAS_VALUE_EXPR_P (var)) ; else if (TREE_STATIC (var)) ; so expects externals/statics after recording alignment. Which means recording alignment is necessary. Note that we also record alignment to make sure we can spill to properly aligned stack slots. I don't see why we don't need to do that for used statics/externs. That is are you sure we never need to spill a var of their type? Richard. > Jan >