Re: [v4] avoid alignment of static variables affecting stack's

[Richard Biener <richard dot guenther at gmail dot com>]

On Mon, Dec 14, 2015 at 9:44 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 14.12.15 at 09:35, <richard.guenther@gmail.com> wrote:
>> On Fri, Dec 11, 2015 at 2:54 PM, Bernd Schmidt <bschmidt@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
>