alignment: store_one_arg vs emit_push_insn

alignment: store_one_arg vs emit_push_insn

[DJ Delorie]

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)

Re: alignment: store_one_arg vs emit_push_insn

[Richard Henderson]

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~

Re: alignment: store_one_arg vs emit_push_insn

[DJ Delorie]

> 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.

Re: alignment: store_one_arg vs emit_push_insn

[Richard Henderson]

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~

Re: alignment: store_one_arg vs emit_push_insn

[DJ Delorie]

> 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.

Re: alignment: store_one_arg vs emit_push_insn

[Richard Henderson]

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~

Re: alignment: store_one_arg vs emit_push_insn

[DJ Delorie]

> 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;

Re: alignment: store_one_arg vs emit_push_insn

[Richard Henderson]

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~

Re: alignment: store_one_arg vs emit_push_insn

[Paul Koning]

> > 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;
> 

Re: alignment: store_one_arg vs emit_push_insn

[Richard Kenner]

      /* 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.

Re: alignment: store_one_arg vs emit_push_insn

[DJ Delorie]

> 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.

Re: alignment: store_one_arg vs emit_push_insn

[Richard Kenner]

    > 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*.

Re: alignment: store_one_arg vs emit_push_insn

[DJ Delorie]

> 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.

Re: alignment: store_one_arg vs emit_push_insn

[Richard Kenner]

    > 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.

Re: alignment: store_one_arg vs emit_push_insn

[DJ Delorie]

> 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.