* [RFC/RFT PATCH, middle end]: Fix PR 34621, [4.3 Regression] gcc.c-torture/execute/va-arg-25.c:32: internal compiler error: in expand_call, at calls.c:2785
@ 2008-02-14 17:41 Uros Bizjak
2008-02-15 8:15 ` Uros Bizjak
0 siblings, 1 reply; 6+ messages in thread
From: Uros Bizjak @ 2008-02-14 17:41 UTC (permalink / raw)
To: GCC Patches; +Cc: Jeffrey Law
[-- Attachment #1: Type: text/plain, Size: 3490 bytes --]
Hello!
This bug is triggered on i686-apple-darwin9, where STACK_BOUNDARY is
fixed to 128, while PARM_BOUNDARY is set to 32 (BITS_PER_WORD).
In gcc.c-torture/execute/va-arg-25.c, a sequence of int/SSE/int/SSE
arguments is pushed to the stack (due to stack-passing ABI for this
particular target). SSE arguments are constrained by their 16 byte
alignment and stack alignment at the call point is forced to 16byte by
STACK_BOUNDARY define.
However, alignment_pad of the argument (this defines the size of the
stack slot of the argument, so _next_ argument is aligned correctly) is
calculated only when (boundary > PARM_BOUNDARY && boundary >
STACK_BOUNDARY). This implies that PARM_BOUNDARY is more or equal to
STACK_BOUNDARY to trigger the condition. In darwin9 case, this is not
true anymore, and alignment_pad of "int" argument remains zero. This
effect can be observed by disabling a couple of checking asserts (see
comment #9 in the PR), in order to inspect wrong call frame for
-mno-accumulate-outgoing-args:
main:
subl $28, %esp
movaps .LC0, %xmm0
movaps %xmm0, (%esp) <- aligned to 16
pushl $2 <- esp = esp - 4
movaps .LC1, %xmm0
subl $16, %esp <- esp = esp - 16
movaps %xmm0, (%esp) <- *crash*
pushl $1 <- esp = esp - 4
call foo <- *not aligned to 16 bytes*
...
When the condition is changed, so it checks if "boundary" is more than
either PARM_BOUNDARY or STACK_BOUNDARY, alignment pad is correctly
calculated, and we compile to:
main:
subl $40, %esp
movaps .LC0, %xmm0
movaps %xmm0, 12(%esp) <- aligned to 16
pushl $2 <- pad (12) + 4
movaps .LC1, %xmm0
subl $28, %esp
movaps %xmm0, 12(%esp) <- aligned to 16
pushl $1 <- pad (12) + 4
call foo <- aligned to 16
...
In -maccumulate-outgoing-args case, we don't use alignment pads, because
gcc constructs call frame using offsets relative to base %esp:
main:
subl $76, %esp
movdqa .LC0, %xmm0
movl $2, 32(%esp)
movdqa %xmm0, 48(%esp)
movl $1, (%esp)
movdqa .LC1, %xmm0
movdqa %xmm0, 16(%esp)
call foo
...
And for x86_64, we use register passing ABI, so this bug is not triggered.
If we don't want STACK_BOUNDARY to represent *HARDWARE* limitation (as
in minimum stack pointer increase by the stack "push" insn) and thus
want to have STACK_BOUNDARY > PARM_BOUNDARY, we need following patch:
2008-02-14 Uros Bizjak <ubizjak@gmail.com>
PR middle-end/34621
* function.c (pad_to_arg_alignment): Calculate alignment_pad if
"boundary" is larger than PARM_BOUNDARY or larger than
STACK_BOUNDARY.
Patch was bootstrapped and regression tested on i686-pc-linux-gnu and
x86_64-pc-linux-gnu. Also, I have compiled and successfully executed
va-arg-25.c with STACK_BOUNDARY set to 128. For added fun, it also works
when integer va-arguments are changed to double, so I'm pretty confident
in this patch.
While I believe that the patch is correct, I don't have
i686-apple-darwin target (or PPC that could also be affected) here to
fully test this change. Volunteers are welcome...
BTW: svn blame attributes Jeffrey Law as the author of this part of
function.c, so I have added him to Cc.
Uros.
[-- Attachment #2: p.diff.txt --]
[-- Type: text/plain, Size: 1251 bytes --]
Index: function.c
===================================================================
--- function.c (revision 132317)
+++ function.c (working copy)
@@ -3450,7 +3450,7 @@ pad_to_arg_alignment (struct args_size *
sp_offset = 0;
#endif
- if (boundary > PARM_BOUNDARY && boundary > STACK_BOUNDARY)
+ if (boundary > PARM_BOUNDARY || boundary > STACK_BOUNDARY)
{
save_var = offset_ptr->var;
save_constant = offset_ptr->constant;
@@ -3476,7 +3476,7 @@ pad_to_arg_alignment (struct args_size *
offset_ptr->var = size_binop (MINUS_EXPR, rounded, sp_offset_tree);
/* ARGS_SIZE_TREE includes constant term. */
offset_ptr->constant = 0;
- if (boundary > PARM_BOUNDARY && boundary > STACK_BOUNDARY)
+ if (boundary > PARM_BOUNDARY || boundary > STACK_BOUNDARY)
alignment_pad->var = size_binop (MINUS_EXPR, offset_ptr->var,
save_var);
}
@@ -3488,7 +3488,7 @@ pad_to_arg_alignment (struct args_size *
#else
CEIL_ROUND (offset_ptr->constant + sp_offset, boundary_in_bytes);
#endif
- if (boundary > PARM_BOUNDARY && boundary > STACK_BOUNDARY)
+ if (boundary > PARM_BOUNDARY || boundary > STACK_BOUNDARY)
alignment_pad->constant = offset_ptr->constant - save_constant;
}
}
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC/RFT PATCH, middle end]: Fix PR 34621, [4.3 Regression] gcc.c-torture/execute/va-arg-25.c:32: internal compiler error: in expand_call, at calls.c:2785
2008-02-14 17:41 [RFC/RFT PATCH, middle end]: Fix PR 34621, [4.3 Regression] gcc.c-torture/execute/va-arg-25.c:32: internal compiler error: in expand_call, at calls.c:2785 Uros Bizjak
@ 2008-02-15 8:15 ` Uros Bizjak
2008-02-15 9:36 ` Uros Bizjak
0 siblings, 1 reply; 6+ messages in thread
From: Uros Bizjak @ 2008-02-15 8:15 UTC (permalink / raw)
To: GCC Patches
On Thu, Feb 14, 2008 at 6:04 PM, Uros Bizjak <ubizjak@gmail.com> wrote:
> Patch was bootstrapped and regression tested on i686-pc-linux-gnu and
> x86_64-pc-linux-gnu. Also, I have compiled and successfully executed
> va-arg-25.c with STACK_BOUNDARY set to 128. For added fun, it also works
> when integer va-arguments are changed to double, so I'm pretty confident
> in this patch.
>
> While I believe that the patch is correct, I don't have
> i686-apple-darwin target (or PPC that could also be affected) here to
> fully test this change. Volunteers are welcome...
To quote Comment #34:
------- Comment #34 From Dominique d'Humieres 2008-02-15 07:21
[reply] -------
With the patch in http://gcc.gnu.org/ml/gcc-patches/2008-02/msg00507.html, the
PR is fixed without regression on intel(32/64 bit) and ppc(32 bit) darwin9.
I'll post the test results ASAP.
---endquote---
OTOH, the test for STACK_BOUNDARY can simply be removed, because
PARM_BOUNDARY is always equal or smaller than STACK_BOUNDARY. I have checked
all targets, and this is true everywhere. So the updated patch:
2008-02-15 Uros Bizjak <ubizjak@gmail.com>
PR middle-end/34621
* function.c (pad_to_arg_alignment): Remove test for STACK_BOUNDARY when
calculating alignment_pad.
OK for mainline?
Uros.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC/RFT PATCH, middle end]: Fix PR 34621, [4.3 Regression] gcc.c-torture/execute/va-arg-25.c:32: internal compiler error: in expand_call, at calls.c:2785
2008-02-15 8:15 ` Uros Bizjak
@ 2008-02-15 9:36 ` Uros Bizjak
2008-02-15 9:45 ` Richard Guenther
0 siblings, 1 reply; 6+ messages in thread
From: Uros Bizjak @ 2008-02-15 9:36 UTC (permalink / raw)
To: GCC Patches
On Fri, Feb 15, 2008 at 9:08 AM, Uros Bizjak <ubizjak@gmail.com> wrote:
> > Patch was bootstrapped and regression tested on i686-pc-linux-gnu and
> > x86_64-pc-linux-gnu. Also, I have compiled and successfully executed
> > va-arg-25.c with STACK_BOUNDARY set to 128. For added fun, it also works
> > when integer va-arguments are changed to double, so I'm pretty confident
> > in this patch.
> >
> > While I believe that the patch is correct, I don't have
> > i686-apple-darwin target (or PPC that could also be affected) here to
> > fully test this change. Volunteers are welcome...
>
> To quote Comment #34:
> With the patch in http://gcc.gnu.org/ml/gcc-patches/2008-02/msg00507.html, the
> PR is fixed without regression on intel(32/64 bit) and ppc(32 bit) darwin9.
> I'll post the test results ASAP.
>
> ---endquote---
>
> OTOH, the test for STACK_BOUNDARY can simply be removed, because
> PARM_BOUNDARY is always equal or smaller than STACK_BOUNDARY. I have checked
> all targets, and this is true everywhere. So the updated patch:
>
> 2008-02-15 Uros Bizjak <ubizjak@gmail.com>
>
> PR middle-end/34621
> * function.c (pad_to_arg_alignment): Remove test for STACK_BOUNDARY when
> calculating alignment_pad.
>
> OK for mainline?
This time with the patch inlined.
Uros.
Index: function.c
===================================================================
--- function.c (revision 132332)
+++ function.c (working copy)
@@ -3450,7 +3450,7 @@ pad_to_arg_alignment (struct args_size *
sp_offset = 0;
#endif
- if (boundary > PARM_BOUNDARY && boundary > STACK_BOUNDARY)
+ if (boundary > PARM_BOUNDARY)
{
save_var = offset_ptr->var;
save_constant = offset_ptr->constant;
@@ -3476,7 +3476,7 @@ pad_to_arg_alignment (struct args_size *
offset_ptr->var = size_binop (MINUS_EXPR, rounded, sp_offset_tree);
/* ARGS_SIZE_TREE includes constant term. */
offset_ptr->constant = 0;
- if (boundary > PARM_BOUNDARY && boundary > STACK_BOUNDARY)
+ if (boundary > PARM_BOUNDARY)
alignment_pad->var = size_binop (MINUS_EXPR, offset_ptr->var,
save_var);
}
@@ -3488,7 +3488,7 @@ pad_to_arg_alignment (struct args_size *
#else
CEIL_ROUND (offset_ptr->constant + sp_offset, boundary_in_bytes);
#endif
- if (boundary > PARM_BOUNDARY && boundary > STACK_BOUNDARY)
+ if (boundary > PARM_BOUNDARY)
alignment_pad->constant = offset_ptr->constant - save_constant;
}
}
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC/RFT PATCH, middle end]: Fix PR 34621, [4.3 Regression] gcc.c-torture/execute/va-arg-25.c:32: internal compiler error: in expand_call, at calls.c:2785
2008-02-15 9:36 ` Uros Bizjak
@ 2008-02-15 9:45 ` Richard Guenther
2008-02-15 9:56 ` Uros Bizjak
0 siblings, 1 reply; 6+ messages in thread
From: Richard Guenther @ 2008-02-15 9:45 UTC (permalink / raw)
To: Uros Bizjak; +Cc: GCC Patches
On Fri, Feb 15, 2008 at 10:31 AM, Uros Bizjak <ubizjak@gmail.com> wrote:
> On Fri, Feb 15, 2008 at 9:08 AM, Uros Bizjak <ubizjak@gmail.com> wrote:
>
> > > Patch was bootstrapped and regression tested on i686-pc-linux-gnu and
> > > x86_64-pc-linux-gnu. Also, I have compiled and successfully executed
> > > va-arg-25.c with STACK_BOUNDARY set to 128. For added fun, it also works
> > > when integer va-arguments are changed to double, so I'm pretty confident
> > > in this patch.
> > >
> > > While I believe that the patch is correct, I don't have
> > > i686-apple-darwin target (or PPC that could also be affected) here to
> > > fully test this change. Volunteers are welcome...
> >
> > To quote Comment #34:
>
>
> > With the patch in http://gcc.gnu.org/ml/gcc-patches/2008-02/msg00507.html, the
> > PR is fixed without regression on intel(32/64 bit) and ppc(32 bit) darwin9.
> > I'll post the test results ASAP.
> >
> > ---endquote---
> >
> > OTOH, the test for STACK_BOUNDARY can simply be removed, because
> > PARM_BOUNDARY is always equal or smaller than STACK_BOUNDARY. I have checked
> > all targets, and this is true everywhere. So the updated patch:
Is this documented? If so ok, otherwise please add an assert at the
top of the function.
Ok with eventually that change.
Thanks,
Richard.
> > 2008-02-15 Uros Bizjak <ubizjak@gmail.com>
> >
> > PR middle-end/34621
> > * function.c (pad_to_arg_alignment): Remove test for STACK_BOUNDARY when
> > calculating alignment_pad.
> >
> > OK for mainline?
>
> This time with the patch inlined.
>
> Uros.
>
> Index: function.c
> ===================================================================
> --- function.c (revision 132332)
> +++ function.c (working copy)
> @@ -3450,7 +3450,7 @@ pad_to_arg_alignment (struct args_size *
> sp_offset = 0;
> #endif
>
> - if (boundary > PARM_BOUNDARY && boundary > STACK_BOUNDARY)
> + if (boundary > PARM_BOUNDARY)
> {
> save_var = offset_ptr->var;
> save_constant = offset_ptr->constant;
> @@ -3476,7 +3476,7 @@ pad_to_arg_alignment (struct args_size *
> offset_ptr->var = size_binop (MINUS_EXPR, rounded, sp_offset_tree);
> /* ARGS_SIZE_TREE includes constant term. */
> offset_ptr->constant = 0;
> - if (boundary > PARM_BOUNDARY && boundary > STACK_BOUNDARY)
> + if (boundary > PARM_BOUNDARY)
> alignment_pad->var = size_binop (MINUS_EXPR, offset_ptr->var,
> save_var);
> }
> @@ -3488,7 +3488,7 @@ pad_to_arg_alignment (struct args_size *
> #else
> CEIL_ROUND (offset_ptr->constant + sp_offset, boundary_in_bytes);
> #endif
> - if (boundary > PARM_BOUNDARY && boundary > STACK_BOUNDARY)
> + if (boundary > PARM_BOUNDARY)
> alignment_pad->constant = offset_ptr->constant - save_constant;
> }
> }
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC/RFT PATCH, middle end]: Fix PR 34621, [4.3 Regression] gcc.c-torture/execute/va-arg-25.c:32: internal compiler error: in expand_call, at calls.c:2785
2008-02-15 9:45 ` Richard Guenther
@ 2008-02-15 9:56 ` Uros Bizjak
2008-02-15 11:03 ` Richard Guenther
0 siblings, 1 reply; 6+ messages in thread
From: Uros Bizjak @ 2008-02-15 9:56 UTC (permalink / raw)
To: Richard Guenther; +Cc: GCC Patches
On Fri, Feb 15, 2008 at 10:35 AM, Richard Guenther
<richard.guenther@gmail.com> wrote:
> On Fri, Feb 15, 2008 at 10:31 AM, Uros Bizjak <ubizjak@gmail.com> wrote:
> > On Fri, Feb 15, 2008 at 9:08 AM, Uros Bizjak <ubizjak@gmail.com> wrote:
> >
> > > > Patch was bootstrapped and regression tested on i686-pc-linux-gnu and
> > > > x86_64-pc-linux-gnu. Also, I have compiled and successfully executed
> > > > va-arg-25.c with STACK_BOUNDARY set to 128. For added fun, it also works
> > > > when integer va-arguments are changed to double, so I'm pretty confident
> > > > in this patch.
> > > >
> > > > While I believe that the patch is correct, I don't have
> > > > i686-apple-darwin target (or PPC that could also be affected) here to
> > > > fully test this change. Volunteers are welcome...
> > >
> > > To quote Comment #34:
> >
> >
> > > With the patch in http://gcc.gnu.org/ml/gcc-patches/2008-02/msg00507.html, the
> > > PR is fixed without regression on intel(32/64 bit) and ppc(32 bit) darwin9.
> > > I'll post the test results ASAP.
> > >
> > > ---endquote---
> > >
> > > OTOH, the test for STACK_BOUNDARY can simply be removed, because
> > > PARM_BOUNDARY is always equal or smaller than STACK_BOUNDARY. I have checked
> > > all targets, and this is true everywhere. So the updated patch:
Documentation says that:
-- Macro: STACK_BOUNDARY
Define this macro to the minimum alignment enforced by hardware
for the stack pointer on this machine. The definition is a C
expression for the desired alignment (measured in bits). This
value is used as a default if `PREFERRED_STACK_BOUNDARY' is not
defined. On most machines, this should be the same as
`PARM_BOUNDARY'.
> Is this documented? If so ok, otherwise please add an assert at the
> top of the function.
OTOH, I think that argument should be aligned according to
PARM_BOUNDARY, and STACK_BOUNDARY IMO just doesn't fit here. So, I
think that assert would be a bit misleading.
Uros.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC/RFT PATCH, middle end]: Fix PR 34621, [4.3 Regression] gcc.c-torture/execute/va-arg-25.c:32: internal compiler error: in expand_call, at calls.c:2785
2008-02-15 9:56 ` Uros Bizjak
@ 2008-02-15 11:03 ` Richard Guenther
0 siblings, 0 replies; 6+ messages in thread
From: Richard Guenther @ 2008-02-15 11:03 UTC (permalink / raw)
To: Uros Bizjak; +Cc: GCC Patches
On Fri, Feb 15, 2008 at 10:44 AM, Uros Bizjak <ubizjak@gmail.com> wrote:
> On Fri, Feb 15, 2008 at 10:35 AM, Richard Guenther
> <richard.guenther@gmail.com> wrote:
> > On Fri, Feb 15, 2008 at 10:31 AM, Uros Bizjak <ubizjak@gmail.com> wrote:
> > > On Fri, Feb 15, 2008 at 9:08 AM, Uros Bizjak <ubizjak@gmail.com> wrote:
> > >
> > > > > Patch was bootstrapped and regression tested on i686-pc-linux-gnu and
> > > > > x86_64-pc-linux-gnu. Also, I have compiled and successfully executed
> > > > > va-arg-25.c with STACK_BOUNDARY set to 128. For added fun, it also works
> > > > > when integer va-arguments are changed to double, so I'm pretty confident
> > > > > in this patch.
> > > > >
> > > > > While I believe that the patch is correct, I don't have
> > > > > i686-apple-darwin target (or PPC that could also be affected) here to
> > > > > fully test this change. Volunteers are welcome...
> > > >
> > > > To quote Comment #34:
> > >
> > >
> > > > With the patch in http://gcc.gnu.org/ml/gcc-patches/2008-02/msg00507.html, the
> > > > PR is fixed without regression on intel(32/64 bit) and ppc(32 bit) darwin9.
> > > > I'll post the test results ASAP.
> > > >
> > > > ---endquote---
> > > >
> > > > OTOH, the test for STACK_BOUNDARY can simply be removed, because
> > > > PARM_BOUNDARY is always equal or smaller than STACK_BOUNDARY. I have checked
> > > > all targets, and this is true everywhere. So the updated patch:
>
> Documentation says that:
>
> -- Macro: STACK_BOUNDARY
> Define this macro to the minimum alignment enforced by hardware
> for the stack pointer on this machine. The definition is a C
> expression for the desired alignment (measured in bits). This
> value is used as a default if `PREFERRED_STACK_BOUNDARY' is not
> defined. On most machines, this should be the same as
> `PARM_BOUNDARY'.
>
>
> > Is this documented? If so ok, otherwise please add an assert at the
> > top of the function.
>
> OTOH, I think that argument should be aligned according to
> PARM_BOUNDARY, and STACK_BOUNDARY IMO just doesn't fit here. So, I
> think that assert would be a bit misleading.
I see. The patch is ok as-is.
Thanks,
Richard.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2008-02-15 9:56 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2008-02-14 17:41 [RFC/RFT PATCH, middle end]: Fix PR 34621, [4.3 Regression] gcc.c-torture/execute/va-arg-25.c:32: internal compiler error: in expand_call, at calls.c:2785 Uros Bizjak
2008-02-15 8:15 ` Uros Bizjak
2008-02-15 9:36 ` Uros Bizjak
2008-02-15 9:45 ` Richard Guenther
2008-02-15 9:56 ` Uros Bizjak
2008-02-15 11:03 ` Richard Guenther
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).