[Bug target/44542] New: expand_one_stack_var_at may set DECL_ALIGN to a too high value

[Bug target/44542] New: expand_one_stack_var_at may set DECL_ALIGN to a too high value

[jakub at gcc dot gnu dot org]

On say -O0:
void foo (long x)
{
  long a, b, c, d;
  __builtin_alloca (1);
}

some of the variables get much higher DECL_ALIGN than they in the end really
have.  I don't have a testcase which shows miscompilation on the trunk, still
it seems to be a latent bug.  IMHO it should instead set DECL_ALIGN to the
maximum of requested DECL_ALIGN and alignment from offset for minimum possible
stack alignment and only after expansion when it is finalized what stack
alignment it will have it can be increased if needed.  On 4.4-RH I've run into
another issue (perhaps also latent on the trunk) - if expand_stack_var is
called twice on the same variable, the first time it is given a stack slot and
DECL_ALIGN set to a very high number (e.g. for offset 32 it is 256 bits) and
next time the function doesn't do almost anything (DECL_RTL_SET_P is true),
except for increasing requested alignment to the given one, so it ends up
wanting to realign the stack.


-- 
           Summary: expand_one_stack_var_at may set DECL_ALIGN to a too high
                    value
           Product: gcc
           Version: 4.6.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
        AssignedTo: unassigned at gcc dot gnu dot org
        ReportedBy: jakub at gcc dot gnu dot org
GCC target triplet: x86_64-linux


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44542

[Bug target/44542] expand_one_stack_var_at may set DECL_ALIGN to a too high value

[matz at gcc dot gnu dot org]

------- Comment #1 from matz at gcc dot gnu dot org  2010-06-15 11:19 -------
We have a slightly problematic ordering issue here.  Here's what I
think should be made the case:
  DECL_ALIGN should not matter after expansion, we have MEM_ALIGN for that.
  (and for calculating offsets from stack-ptr we can calculate the
   alignment directly)

If that were the case already we wouldn't have the problem of having to
maintain DECL_ALIGN.  Of course the problem is, that MEM_ALIGN is actually
generated from DECL_ALIGN during expansion itself.  It follows that it
actually wouldn't help at all to fixup only DECL_ALIGN after the final
stack alignment is known.  We would also have to walk all RTL and fixup
MEM_ALIGN too (in possibly non-obvious ways).  If we wouldn't do that
but start with minimal DECL_ALIGN we have only managed to produce very
suboptimal code.  On some architecture even so unoptimal as to use unaligned
instructions were aligned ones would be possible.  And we wouldn't necessarily
be able to fixup _that_ after the fact.

Now, the mentioned ordering problem is, that we align the stack only after
all instructions are converted (and hence all stack-vars are expanded).  We
can't do it before, because the necessity for stack-alignment depends on
the actually emitted stack-vars.  And doing it afterwards will lead to the 
need for walking all instructions again, fixing DECL_ALIGN and MEM_ALIGN
(and changing instructions to use more optimal versions of the alignment
now is somehow better).

I think the only correct way is for expand_stack_alignment to align the
stack exactly so that all DECL_ALIGN and MEM_ALIGN entries are correct.
In other words I think it would be a bug for expand_stack_alignment to
generate an actual stack alignment that would lessen the alignment of
stack vars.

To that effect I think I'd rather want to see a reproducer for 4.5/4.6
and fix the bug in expand_stack_alignment (possibly in the target hooks)
than trying to fiddle with the insn chain.

Weren't there some DRAP fixes after 4.4?  I seem to remember some patches
flying by at that time-frame.  Perhaps you can also create a reproducer
for 4.5 just before expand-from-ssa?


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44542

[Bug target/44542] expand_one_stack_var_at may set DECL_ALIGN to a too high value

[jakub at gcc dot gnu dot org]

------- Comment #2 from jakub at gcc dot gnu dot org  2010-06-15 11:36 -------
I don't have any wrong-code testcases.
__attribute__((noinline, noclone)) void
f (long x)
{
  long a, b, c, d;
  asm ("");
  __builtin_alloca (1);
}

int
main (void)
{
  f (1234567890);
  return 0;
}

shows the same issue at -O0 -g as I see on redhat/gcc-4_4-branch on the trunk
between r145138 (which I've backported to 4.4-RH, perhaps I'll need to back it
out) and r146817 (SSA expand).  In between those two commits the trunk first
sets DECL_ALIGN to 256 bits on one of the variables (as offset 32 gives and is
still smaller or equal to MAX_SUPPORTED_STACK_ALIGNMENT) and then
expand_stack_var is called on it again and bumps
crtl->stack_alignment_estimated
to 256 (something that isn't really needed).


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44542

[Bug target/44542] expand_one_stack_var_at may set DECL_ALIGN to a too high value

[jakub at gcc dot gnu dot org]

------- Comment #3 from jakub at gcc dot gnu dot org  2010-06-15 12:53 -------
Created an attachment (id=20914)
 --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=20914&action=view)
gcc46-pr44542.patch

For the "don't use changed DECL_ALIGN for stack estimation after
expand_one_stack_var_at has been called" subprogram we can do something like
this.

For the other issue, I wonder if we couldn't limit align in
expand_one_stack_var_at to minimum of MAX_SUPPORTED_STACK_ALIGNMENT
and crtl->stack_alignment_estimated or something similar.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44542

[Bug target/44542] expand_one_stack_var_at may set DECL_ALIGN to a too high value

[matz at gcc dot gnu dot org]

------- Comment #4 from matz at gcc dot gnu dot org  2010-06-15 13:40 -------
Can you try to instead do the stack-estimation only when really_expand is
false?  The issue is, we see all variables (or we _should_ see) exactly twice,
once for estimation, once for generating the DECL_RTL.  The code was so
twisted that I didn't want to touch it too much during expand-from-ssa, but
I planned to return to that somewhen, hence I'm not sure if we really see
each variable only twice.  But we should be working towards that goal.

In any case it should be fine to track crtl->stack_alignment_estimated only
in the first pass (really_expand == false), and never touch it again in
the second pass (really_expand == true).  Then you should also be able
to simplify the condition.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44542

[Bug target/44542] expand_one_stack_var_at may set DECL_ALIGN to a too high value

[matz at gcc dot gnu dot org]

------- Comment #5 from matz at gcc dot gnu dot org  2010-06-15 13:50 -------
Oh, and yes, I agree that in expand_one_stack_var_at (only called when
really_expand == true) we should limit align to $something.  I'm just not
sure what $something is.  crtl->stack_alignment_estimated is not really it,
because that one itself is adjusted by expand_stack_alignment (running after
all expansion).


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44542

[Bug target/44542] expand_one_stack_var_at may set DECL_ALIGN to a too high value

[hjl dot tools at gmail dot com]

------- Comment #6 from hjl dot tools at gmail dot com  2010-06-15 14:46 -------
I watched crtl->stack_alignment_estimated in gdb
with the testcase in comment #2. I didn't see it
set to 256.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44542

[Bug target/44542] expand_one_stack_var_at may set DECL_ALIGN to a too high value

[matz at gcc dot gnu dot org]

------- Comment #7 from matz at gcc dot gnu dot org  2010-06-15 14:56 -------
Jakub was not talking about crtl->stack_alignment_estimated becoming 256,
but rather DECL_ALIGN of certain decls in expand_one_stack_var_at.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44542

[Bug target/44542] expand_one_stack_var_at may set DECL_ALIGN to a too high value

[hjl dot tools at gmail dot com]

------- Comment #8 from hjl dot tools at gmail dot com  2010-06-15 15:39 -------
(In reply to comment #7)
> Jakub was not talking about crtl->stack_alignment_estimated becoming 256,
> but rather DECL_ALIGN of certain decls in expand_one_stack_var_at.
> 

I set the breakpoint at expand_one_stack_var_at and ran
the testcase in gdb. expand_one_stack_var_at wasn't called.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44542

[Bug target/44542] expand_one_stack_var_at may set DECL_ALIGN to a too high value

[jakub at gcc dot gnu dot org]

------- Comment #9 from jakub at gcc dot gnu dot org  2010-06-15 16:45 -------
Re: #c4 - !really_expand never occur when !optimize and for optimize they
are called IMHO way too early (during inlining etc.).

Re: #c8 - the testcases were meant for the given range of svn revs of trunk to
show the issue where expand_one_var is called twice and that forces useless
realignment.  If you want just see too high DECL_ALIGN setting, try:
void
f (long x)
{
  long a, b, c, d;
  asm ("" : : "r" (&a), "r" (&b), "r" (&c), "r" (&d));
  __builtin_alloca (1);
}

at any optimization level on x86_64-linux on the trunk.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44542

[Bug target/44542] expand_one_stack_var_at may set DECL_ALIGN to a too high value

[hjl dot tools at gmail dot com]

------- Comment #10 from hjl dot tools at gmail dot com  2010-06-15 17:13 -------
Created an attachment (id=20920)
 --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=20920&action=view)
A patch to use alignment

If we already know the alignment we need, why not use it? Here is
a patch does it.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44542

[Bug target/44542] expand_one_stack_var_at may set DECL_ALIGN to a too high value

[hjl dot tools at gmail dot com]

------- Comment #11 from hjl dot tools at gmail dot com  2010-06-15 17:20 -------
Created an attachment (id=20921)
 --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=20921&action=view)
An updated patch

I don't see why expand_one_stack_var_at should compute alignment
when its callers know what the alignment should be. We just need
to do some sanity check.


-- 

hjl dot tools at gmail dot com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #20920|0                           |1
        is obsolete|                            |


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44542

[Bug target/44542] expand_one_stack_var_at may set DECL_ALIGN to a too high value

[hjl dot tools at gmail dot com]

------- Comment #12 from hjl dot tools at gmail dot com  2010-06-15 17:25 -------
Created an attachment (id=20922)
 --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=20922&action=view)
A new patch

Fix typo and update comments.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44542

[Bug target/44542] expand_one_stack_var_at may set DECL_ALIGN to a too high value

[jakub at gcc dot gnu dot org]

------- Comment #13 from jakub at gcc dot gnu dot org  2010-06-16 06:55 -------
This looks wrong.  The code in expand_one_stack_var_at (before dynamic stack
realignment) made perfect sense, if we gave a bigger alignment to some variable
(e.g. automatic array), it is useful to tell the expanders that we did.
And if it got a smaller than requested alignment, similarly it is better to
tell that to the expanders (the latter happens when the requested alignment is
larger than what can be supported for automatic vars).
>From the offset we don't know absolute alignment, unless we know how aligned is
virtual_stack_vars_rtx.  We should know that without dynamic stack realignment,
for dynamic stack realignment all we need to ensure is that the computed
alignment isn't larger than the one we are going to use.
E.g. replacing MAX_SUPPORTED_STACK_ALIGNMENT with
crtl->max_used_stack_slot_alignment (or crtl->stack_alignment_needed).
Or say MAX (crtl->max_used_stack_slot_alignment, PREFERRED_STACK_BOUNDARY)
or MAX (crtl->stack_alignment_needed, PREFERRED_STACK_BOUNDARY)?


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44542

[Bug target/44542] expand_one_stack_var_at may set DECL_ALIGN to a too high value

[hjl dot tools at gmail dot com]

------- Comment #14 from hjl dot tools at gmail dot com  2010-06-16 14:36 -------
The code in question is

      offset -= frame_phase;
      align = offset & -offset;
      align *= BITS_PER_UNIT;
      if (align == 0)
        align = STACK_BOUNDARY;
      else if (align > MAX_SUPPORTED_STACK_ALIGNMENT)
        align = MAX_SUPPORTED_STACK_ALIGNMENT;

offset is computed from

  align = get_decl_align_unit (SSAVAR (var));
  offset = alloc_stack_frame_space (size, align);

If you compute alignment from offset again, you
will get a number >= requested alignment. That is
when we allocate 8byte aligned at 8byte from stack,
we may get offset 8, 16, 24, 32, 40, 48, 56, 64,
... depending on how much we have allocated on stack.

As for MAX_SUPPORTED_STACK_ALIGNMENT, I doubt we
will hit it since it is a huge number.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44542

[Bug target/44542] expand_one_stack_var_at may set DECL_ALIGN to a too high value

[jakub at gcc dot gnu dot org]

------- Comment #15 from jakub at gcc dot gnu dot org  2010-06-16 14:57 -------
1) all the world isn't ix86/x86_64, this is generic code, so
MAX_SUPPORTED_STACK_ALIGNMENT is small on many targets
2) even when get_decl_align_unit returns something small, the decl might still
get a nicely aligned slot (say offset 64).  If it is known at this point
that virtual_stack_vars_rtx will be at least 32 byte aligned, we shouldn't hide
that fact from the expanders.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44542

[Bug target/44542] expand_one_stack_var_at may set DECL_ALIGN to a too high value

[hjl dot tools at gmail dot com]

------- Comment #16 from hjl dot tools at gmail dot com  2010-06-16 15:38 -------
(In reply to comment #15)
> 2) even when get_decl_align_unit returns something small, the decl might still
> get a nicely aligned slot (say offset 64).  If it is known at this point
> that virtual_stack_vars_rtx will be at least 32 byte aligned, we shouldn't hide
> that fact from the expanders.
> 

I just pointed out where "expand_one_stack_var_at may set DECL_ALIGN to
a too high value" came from. To guarantee "virtual_stack_vars_rtx will
be at least 32 byte aligned", stack has to be aligned at 32byte to begin
with. Otherwise, you may not get 32byte alignment.

The current code looks correct to me if you want to use stack offset
as alignment.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44542

[Bug target/44542] expand_one_stack_var_at may set DECL_ALIGN to a too high value

[jakub at gcc dot gnu dot org]

------- Comment #17 from jakub at gcc dot gnu dot org  2010-07-27 17:55 -------
Subject: Bug 44542

Author: jakub
Date: Tue Jul 27 17:54:32 2010
New Revision: 162582

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=162582
Log:
        PR target/44542
        * cfgexpand.c (expand_one_stack_var_at): Limit align to maximum
        of max_used_stack_slot_alignment and PREFERRED_STACK_BOUNDARY
        instead of MAX_SUPPORTED_STACK_ALIGNMENT.
        (expand_one_var): Don't consider DECL_ALIGN for variables for
        which expand_one_stack_var_at has been already called.

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/cfgexpand.c


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44542

[Bug target/44542] expand_one_stack_var_at may set DECL_ALIGN to a too high value

[jakub at gcc dot gnu dot org]

------- Comment #18 from jakub at gcc dot gnu dot org  2010-07-27 17:59 -------
Fixed.


-- 

jakub at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |RESOLVED
         Resolution|                            |FIXED


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44542

[Bug target/44542] expand_one_stack_var_at may set DECL_ALIGN to a too high value

[hjl dot tools at gmail dot com]

------- Comment #19 from hjl dot tools at gmail dot com  2010-09-17 20:24 -------
It comes back with revision 164375:

http://gcc.gnu.org/ml/gcc-cvs/2010-09/msg00669.html

for PR 45678. On Linux/ia32, I got

FAIL: gcc.target/i386/incoming-9.c scan-assembler-not andl[\\t ]*\\$-16,[\\t
]*%esp

It is because we are using stack offset of local variable for its
alignment.


-- 

hjl dot tools at gmail dot com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
OtherBugsDependingO|                            |45678
              nThis|                            |
             Status|RESOLVED                    |UNCONFIRMED
         Resolution|FIXED                       |


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44542