[Bug target/44542] expand_one_stack_var_at may set DECL_ALIGN to a too high value
--- 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
[Bug target/44542] expand_one_stack_var_at may set DECL_ALIGN to a too high value
--- 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=gccview=revrev=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
--- 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
--- 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
--- 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
--- 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
--- 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
--- 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
--- 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
--- 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=20914action=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
--- 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
--- 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
--- 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
--- 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
--- 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
--- 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
--- 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=20920action=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
--- 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=20921action=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
--- 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=20922action=view) A new patch Fix typo and update comments. -- http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44542
