* Re: alignment: store_one_arg vs emit_push_insn
@ 2003-05-10 2:24 Richard Kenner
2003-05-11 2:22 ` DJ Delorie
0 siblings, 1 reply; 15+ messages in thread
From: Richard Kenner @ 2003-05-10 2:24 UTC (permalink / raw)
To: dj; +Cc: gcc
/* Some types will require stricter alignment, which will be
provided for elsewhere in argument layout. */
parm_align = MAX (PARM_BOUNDARY, TYPE_ALIGN (TREE_TYPE (pval)));
It would seem to me that the MAX should in fact be a MIN in
store_one_arg.
No, it looks right to me. PARM_BOUNDARY is the smallest alignment a
parameter is allowed to have, but some might be more aligned.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: alignment: store_one_arg vs emit_push_insn
2003-05-10 2:24 alignment: store_one_arg vs emit_push_insn Richard Kenner
@ 2003-05-11 2:22 ` DJ Delorie
0 siblings, 0 replies; 15+ messages in thread
From: DJ Delorie @ 2003-05-11 2:22 UTC (permalink / raw)
To: kenner; +Cc: gcc
> No, it looks right to me. PARM_BOUNDARY is the smallest alignment a
> parameter is allowed to have, but some might be more aligned.
Then emit_push_insn is misusing it, as it uses it as the alignment of
the source, not just the destination.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: alignment: store_one_arg vs emit_push_insn
2003-05-12 22:47 ` DJ Delorie
2003-05-12 23:14 ` Richard Henderson
@ 2003-05-13 1:04 ` Paul Koning
1 sibling, 0 replies; 15+ messages in thread
From: Paul Koning @ 2003-05-13 1:04 UTC (permalink / raw)
To: dj; +Cc: rth, gcc
> > On Mon, May 12, 2003 at 02:16:49PM -0400, DJ Delorie wrote:
> > > Would it be sufficient if it just
> > > chose the minimum of the available alignment? If so, then most of the
> > > "reorg" would just be commenting the new semantics.
> >
> > Yes, that would be fine.
>
> How does this look? The tests are still running, but so far no regressions.
Doesn't that do the *maximum* of the alignments? I would think
minimum, as you said, is what you'd want instead.
paul
> 2003-05-12 DJ Delorie <dj@redhat.com>
>
> * expr.c (move_by_pieces): Honor the alignment inherent in the
> source and destination if available.
>
> Index: expr.c
> ===================================================================
> RCS file: /cvs/uberbaum/gcc/expr.c,v
> retrieving revision 1.538
> diff -p -2 -r1.538 expr.c
> *** expr.c 9 May 2003 06:37:18 -0000 1.538
> --- expr.c 12 May 2003 22:43:24 -0000
> *************** convert_modes (mode, oldmode, x, unsigne
> *** 1464,1468 ****
> used to push FROM to the stack.
>
> ! ALIGN is maximum alignment we can assume. */
>
> void
> --- 1464,1468 ----
> used to push FROM to the stack.
>
> ! ALIGN is maximum stack alignment we can assume. */
>
> void
> *************** move_by_pieces (to, from, len, align)
> *** 1477,1480 ****
> --- 1477,1485 ----
> enum machine_mode mode = VOIDmode, tmode;
> enum insn_code icode;
> +
> + if (to && MEM_ALIGN (to) > 0)
> + align = MEM_ALIGN (to);
> + if (align < MEM_ALIGN (from) && MEM_ALIGN (from) > 0)
> + align = MEM_ALIGN (from);
>
> data.offset = 0;
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: alignment: store_one_arg vs emit_push_insn
2003-05-12 22:47 ` DJ Delorie
@ 2003-05-12 23:14 ` Richard Henderson
2003-05-13 1:04 ` Paul Koning
1 sibling, 0 replies; 15+ messages in thread
From: Richard Henderson @ 2003-05-12 23:14 UTC (permalink / raw)
To: DJ Delorie; +Cc: gcc
On Mon, May 12, 2003 at 06:47:13PM -0400, DJ Delorie wrote:
> * expr.c (move_by_pieces): Honor the alignment inherent in the
> source and destination if available.
Looks ok.
r~
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: alignment: store_one_arg vs emit_push_insn
2003-05-12 19:41 ` Richard Henderson
@ 2003-05-12 22:47 ` DJ Delorie
2003-05-12 23:14 ` Richard Henderson
2003-05-13 1:04 ` Paul Koning
0 siblings, 2 replies; 15+ messages in thread
From: DJ Delorie @ 2003-05-12 22:47 UTC (permalink / raw)
To: rth; +Cc: gcc
> On Mon, May 12, 2003 at 02:16:49PM -0400, DJ Delorie wrote:
> > Would it be sufficient if it just
> > chose the minimum of the available alignment? If so, then most of the
> > "reorg" would just be commenting the new semantics.
>
> Yes, that would be fine.
How does this look? The tests are still running, but so far no regressions.
2003-05-12 DJ Delorie <dj@redhat.com>
* expr.c (move_by_pieces): Honor the alignment inherent in the
source and destination if available.
Index: expr.c
===================================================================
RCS file: /cvs/uberbaum/gcc/expr.c,v
retrieving revision 1.538
diff -p -2 -r1.538 expr.c
*** expr.c 9 May 2003 06:37:18 -0000 1.538
--- expr.c 12 May 2003 22:43:24 -0000
*************** convert_modes (mode, oldmode, x, unsigne
*** 1464,1468 ****
used to push FROM to the stack.
! ALIGN is maximum alignment we can assume. */
void
--- 1464,1468 ----
used to push FROM to the stack.
! ALIGN is maximum stack alignment we can assume. */
void
*************** move_by_pieces (to, from, len, align)
*** 1477,1480 ****
--- 1477,1485 ----
enum machine_mode mode = VOIDmode, tmode;
enum insn_code icode;
+
+ if (to && MEM_ALIGN (to) > 0)
+ align = MEM_ALIGN (to);
+ if (align < MEM_ALIGN (from) && MEM_ALIGN (from) > 0)
+ align = MEM_ALIGN (from);
data.offset = 0;
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: alignment: store_one_arg vs emit_push_insn
2003-05-12 18:19 ` DJ Delorie
@ 2003-05-12 19:41 ` Richard Henderson
2003-05-12 22:47 ` DJ Delorie
0 siblings, 1 reply; 15+ messages in thread
From: Richard Henderson @ 2003-05-12 19:41 UTC (permalink / raw)
To: DJ Delorie; +Cc: gcc
On Mon, May 12, 2003 at 02:16:49PM -0400, DJ Delorie wrote:
> Would it be sufficient if it just
> chose the minimum of the available alignment? If so, then most of the
> "reorg" would just be commenting the new semantics.
Yes, that would be fine.
r~
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: alignment: store_one_arg vs emit_push_insn
2003-05-12 3:54 ` Richard Henderson
@ 2003-05-12 18:19 ` DJ Delorie
2003-05-12 19:41 ` Richard Henderson
0 siblings, 1 reply; 15+ messages in thread
From: DJ Delorie @ 2003-05-12 18:19 UTC (permalink / raw)
To: rth; +Cc: gcc
> Clearly this is all confused. There's no need to glean the value
> of the argument from a passed in "align" parameter. If the value
> is in memory then MEM_ALIGN will contain the proper alignment.
> True, MEM_ALIGN did not exist when this routine was written, but
> that doesn't mean we can't clean this up now.
So, what would a minimal fix be? Ignore align and read MEM_ALIGN from
the parameters?
> I think both move_by_pieces and emit_push_insn should be reorged
> and clarified that the alignment parameter applies only to the
> stack (i.e. to==NULL in the move_by_pieces case).
move_by_pieces is fairly naive about mem-mem transfers. It doesn't
look like it would be able to take advantage of knowing both
alignments without a large change. Would it be sufficient if it just
chose the minimum of the available alignment? If so, then most of the
"reorg" would just be commenting the new semantics.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: alignment: store_one_arg vs emit_push_insn
2003-05-10 0:27 ` DJ Delorie
@ 2003-05-12 3:54 ` Richard Henderson
2003-05-12 18:19 ` DJ Delorie
0 siblings, 1 reply; 15+ messages in thread
From: Richard Henderson @ 2003-05-12 3:54 UTC (permalink / raw)
To: DJ Delorie; +Cc: gcc
On Fri, May 09, 2003 at 08:27:56PM -0400, DJ Delorie wrote:
> The problem is that the alignment is used to choose a move_by_pieces
> mode, and HImode is chosen, and the object we're copying *from* is not
> HI-aligned.
Clearly this is all confused. There's no need to glean the value
of the argument from a passed in "align" parameter. If the value
is in memory then MEM_ALIGN will contain the proper alignment.
True, MEM_ALIGN did not exist when this routine was written, but
that doesn't mean we can't clean this up now.
I think both move_by_pieces and emit_push_insn should be reorged
and clarified that the alignment parameter applies only to the
stack (i.e. to==NULL in the move_by_pieces case).
r~
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: alignment: store_one_arg vs emit_push_insn
2003-05-11 14:49 Richard Kenner
@ 2003-05-11 15:30 ` DJ Delorie
0 siblings, 0 replies; 15+ messages in thread
From: DJ Delorie @ 2003-05-11 15:30 UTC (permalink / raw)
To: kenner; +Cc: gcc
> I thought you were talking about it using PARM_BOUNDARY.
>
> In this case the bug seems to be that xinner isn't properly aligned.
The testcase is gcc.dg/struct-by-value-1.c
"xinner" is originally an struct with an array of char, 8-bit aligned,
at an odd address. That is "properly aligned" for that type. The
stack and parameters on it are 16-bit aligned.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: alignment: store_one_arg vs emit_push_insn
@ 2003-05-11 14:49 Richard Kenner
2003-05-11 15:30 ` DJ Delorie
0 siblings, 1 reply; 15+ messages in thread
From: Richard Kenner @ 2003-05-11 14:49 UTC (permalink / raw)
To: dj; +Cc: gcc
> I don't follow. emit_push_insn barely uses it *at all*.
But the one place it *does* use it is here:
move_by_pieces (NULL, xinner, INTVAL (size) - used, align);
In the failing case, align is 16 and xinner is 8-bit aligned.
I thought you were talking about it using PARM_BOUNDARY.
In this case the bug seems to be that xinner isn't properly aligned.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: alignment: store_one_arg vs emit_push_insn
2003-05-11 11:31 Richard Kenner
@ 2003-05-11 14:33 ` DJ Delorie
0 siblings, 0 replies; 15+ messages in thread
From: DJ Delorie @ 2003-05-11 14:33 UTC (permalink / raw)
To: kenner; +Cc: gcc
> I don't follow. emit_push_insn barely uses it *at all*.
But the one place it *does* use it is here:
move_by_pieces (NULL, xinner, INTVAL (size) - used, align);
In the failing case, align is 16 and xinner is 8-bit aligned.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: alignment: store_one_arg vs emit_push_insn
@ 2003-05-11 11:31 Richard Kenner
2003-05-11 14:33 ` DJ Delorie
0 siblings, 1 reply; 15+ messages in thread
From: Richard Kenner @ 2003-05-11 11:31 UTC (permalink / raw)
To: dj; +Cc: gcc
> No, it looks right to me. PARM_BOUNDARY is the smallest alignment a
> parameter is allowed to have, but some might be more aligned.
Then emit_push_insn is misusing it, as it uses it as the alignment of
the source, not just the destination.
I don't follow. emit_push_insn barely uses it *at all*.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: alignment: store_one_arg vs emit_push_insn
2003-05-10 0:05 ` Richard Henderson
@ 2003-05-10 0:27 ` DJ Delorie
2003-05-12 3:54 ` Richard Henderson
0 siblings, 1 reply; 15+ messages in thread
From: DJ Delorie @ 2003-05-10 0:27 UTC (permalink / raw)
To: rth; +Cc: gcc
> MAX is correct in this case because we know that we're storing
> to a parameter, and we know that PARM_BOUNDARY is the minimum
> alignment of any parameter.
The problem is that the alignment is used to choose a move_by_pieces
mode, and HImode is chosen, and the object we're copying *from* is not
HI-aligned.
There are two alignments, the alignment of the object on the stack,
and the alignment of the object we're copying from. parm_align is
computed as if it were the stack alignment, but used as if it were the
"from" alignment also.
> > The way the code is now, xstormy16 is trying to access
> > a byte-aligned object with a HImode move, and failing
>
> You don't give enough information to determine why this might
> be the case.
parm_align is used here:
emit_push_insn (arg->value, arg->mode, TREE_TYPE (pval), size_rtx,
parm_align, partial, reg, excess, argblock,
ARGS_SIZE_RTX (arg->locate.offset), reg_parm_stack_space,
ARGS_SIZE_RTX (arg->locate.alignment_pad));
The comments in front of emit_push_insn state:
ALIGN (in bits) is maximum alignment we can assume.
and later used here:
move_by_pieces (NULL, xinner, INTVAL (size) - used, align);
In the xstormy16 case, parm_align is 16 (the stack is HI-aligned), but
the array we're pushing on the stack is QI-aligned (the tree for it
has align=8). But we end up passing "16" to move_by_pieces, which is
wrong.
It looks like emit_push_insn does *not* want the alignment on the
stack, or at least needs both the desired final alignment *and* the
worst-case alignment. Or at least emit_push_insn needs to check the
alignment of the type tree passed to it, and do the right thing wrt
move_by_pieces.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: alignment: store_one_arg vs emit_push_insn
2003-05-09 23:34 DJ Delorie
@ 2003-05-10 0:05 ` Richard Henderson
2003-05-10 0:27 ` DJ Delorie
0 siblings, 1 reply; 15+ messages in thread
From: Richard Henderson @ 2003-05-10 0:05 UTC (permalink / raw)
To: DJ Delorie; +Cc: gcc
On Fri, May 09, 2003 at 07:34:36PM -0400, DJ Delorie wrote:
> /* Some types will require stricter alignment, which will be
> provided for elsewhere in argument layout. */
> parm_align = MAX (PARM_BOUNDARY, TYPE_ALIGN (TREE_TYPE (pval)));
MAX is correct in this case because we know that we're storing
to a parameter, and we know that PARM_BOUNDARY is the minimum
alignment of any parameter.
> The way the code is now, xstormy16 is trying to access
> a byte-aligned object with a HImode move, and failing
You don't give enough information to determine why this might
be the case.
r~
^ permalink raw reply [flat|nested] 15+ messages in thread
* alignment: store_one_arg vs emit_push_insn
@ 2003-05-09 23:34 DJ Delorie
2003-05-10 0:05 ` Richard Henderson
0 siblings, 1 reply; 15+ messages in thread
From: DJ Delorie @ 2003-05-09 23:34 UTC (permalink / raw)
To: gcc
In store_one_arg (calls.c) we see this code, computing the alignment
of a parameter:
/* Some types will require stricter alignment, which will be
provided for elsewhere in argument layout. */
parm_align = MAX (PARM_BOUNDARY, TYPE_ALIGN (TREE_TYPE (pval)));
But, in emit_push_insn where that alignment is used (it's not used
anywhere else), we see this comment:
ALIGN (in bits) is maximum alignment we can assume.
It would seem to me that the MAX should in fact be a MIN in
store_one_arg. The way the code is now, xstormy16 is trying to access
a byte-aligned object with a HImode move, and failing
(STRICT_ALIGNMENT is set).
Either that or these two functions just aren't communicating well ;-)
(Or I'm not understanding what's supposed to be happening here, of course)
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2003-05-13 1:04 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2003-05-10 2:24 alignment: store_one_arg vs emit_push_insn Richard Kenner
2003-05-11 2:22 ` DJ Delorie
-- strict thread matches above, loose matches on Subject: below --
2003-05-11 14:49 Richard Kenner
2003-05-11 15:30 ` DJ Delorie
2003-05-11 11:31 Richard Kenner
2003-05-11 14:33 ` DJ Delorie
2003-05-09 23:34 DJ Delorie
2003-05-10 0:05 ` Richard Henderson
2003-05-10 0:27 ` DJ Delorie
2003-05-12 3:54 ` Richard Henderson
2003-05-12 18:19 ` DJ Delorie
2003-05-12 19:41 ` Richard Henderson
2003-05-12 22:47 ` DJ Delorie
2003-05-12 23:14 ` Richard Henderson
2003-05-13 1:04 ` Paul Koning
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).