"H.J. Lu" <hjl.to...@gmail.com> writes:
> On Thu, Jul 3, 2025 at 11:02 PM Richard Sandiford
> <richard.sandif...@arm.com> wrote:
>>
>> "H.J. Lu" <hjl.to...@gmail.com> writes:
>> > Since a backend may ignore user type alignment for arguments passed on
>> > stack, update alignment for arguments passed on stack when copying MEM's
>> > memory attributes.
>> >
>> > gcc/
>> >
>> > PR target/120839
>> > * emit-rtl.cc (set_mem_attrs): Update alignment for argument on
>> > stack.
>> >
>> > gcc/testsuite/
>> >
>> > PR target/120839
>> > * gcc.target/i386/pr120839-1.c: New test.
>> > * gcc.target/i386/pr120839-2.c: Likewise.
>> >
>> >
>> > --
>> > H.J.
>> >
>> > From 3f8a9bfb4beae47bfc0da20b517a5b3b06a1cbcc Mon Sep 17 00:00:00 2001
>> > From: "H.J. Lu" <hjl.to...@gmail.com>
>> > Date: Sat, 28 Jun 2025 06:27:25 +0800
>> > Subject: [PATCH] Update alignment for argument on stack
>> >
>> > Since a backend may ignore user type alignment for arguments passed on
>> > stack, update alignment for arguments passed on stack when copying MEM's
>> > memory attributes.
>> >
>> > gcc/
>> >
>> > PR target/120839
>> > * emit-rtl.cc (set_mem_attrs): Update alignment for argument on
>> > stack.
>> >
>> > gcc/testsuite/
>> >
>> > PR target/120839
>> > * gcc.target/i386/pr120839-1.c: New test.
>> > * gcc.target/i386/pr120839-2.c: Likewise.
>> >
>> > Signed-off-by: H.J. Lu <hjl.to...@gmail.com>
>> > ---
>> > gcc/emit-rtl.cc | 14 ++++++++++++++
>> > gcc/testsuite/gcc.target/i386/pr120839-1.c | 14 ++++++++++++++
>> > gcc/testsuite/gcc.target/i386/pr120839-2.c | 19 +++++++++++++++++++
>> > 3 files changed, 47 insertions(+)
>> > create mode 100644 gcc/testsuite/gcc.target/i386/pr120839-1.c
>> > create mode 100644 gcc/testsuite/gcc.target/i386/pr120839-2.c
>> >
>> > diff --git a/gcc/emit-rtl.cc b/gcc/emit-rtl.cc
>> > index f4fc92bb37a..0d1616361ca 100644
>> > --- a/gcc/emit-rtl.cc
>> > +++ b/gcc/emit-rtl.cc
>> > @@ -389,6 +389,20 @@ set_mem_attrs (rtx mem, mem_attrs *attrs)
>> > {
>> > MEM_ATTRS (mem) = ggc_alloc<mem_attrs> ();
>> > memcpy (MEM_ATTRS (mem), attrs, sizeof (mem_attrs));
>> > + if (MEM_EXPR (mem))
>> > + {
>> > + tree base_address = get_base_address (MEM_EXPR (mem));
>> > + if (base_address && TREE_CODE (base_address) == PARM_DECL)
>> > + {
>> > + /* User alignment on type may be ignored for parameter
>> > + passed on stack. */
>> > + tree type = TREE_TYPE (base_address);
>> > + unsigned int alignment
>> > + = targetm.calls.function_arg_boundary (TYPE_MODE (type),
>> > + type);
>> > + set_mem_align (mem, alignment);
>> > + }
>> > + }
>> > }
>> > }
>>
>> This doesn't feel like the right place to address this. set_mem_attrs
>> is just supposed to install the attributes that it has been given,
>> without second-guessing the contents.
>>
>> Where does the incorrect alignment ultimately come from? (As in,
>> which piece of code creates the MEM and fails to give it the correct
>> alignment?)
>
> x86 has
>
> static unsigned int
> ix86_function_arg_boundary (machine_mode mode, const_tree type)
> {
> unsigned int align;
> if (type)
> {
> /* Since the main variant type is used for call, we convert it to
> the main variant type. */
> type = TYPE_MAIN_VARIANT (type);
> align = TYPE_ALIGN (type);
> if (TYPE_EMPTY_P (type))
> return PARM_BOUNDARY;
> }
>
> Because of
>
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=120900
>
> for C,
>
> typedef struct {
> long double a, b
> } c __attribute__((aligned(32)));
>
> when such a variable is passed on stack, its alignment
> on stack is 16, not 32. However, when set_mem_attrs
^^^^^^^^^^^^^^^^^^
> is called to set MEM_ALIGN for such a variable on stack,
^^^^^^^^^^^^^^^^^^^^^^^^^^
> it sets MEM_ALIGN to 32, instead of 16.
But what I meant was: which piece of code calls set_mem_attrs
to set MEM_ALIGN in this case? The bug seems further up the
call stack to me.
Richard
> But 16 is the
> actual alignment on stack. This means that the incorrect
> alignment is set exactly where my patch tries to change.
>
>> Richard
>>
>> >
>> > diff --git a/gcc/testsuite/gcc.target/i386/pr120839-1.c
>> > b/gcc/testsuite/gcc.target/i386/pr120839-1.c
>> > new file mode 100644
>> > index 00000000000..74fbf876330
>> > --- /dev/null
>> > +++ b/gcc/testsuite/gcc.target/i386/pr120839-1.c
>> > @@ -0,0 +1,14 @@
>> > +/* { dg-do compile } */
>> > +/* { dg-options "-O2" } */
>> > +
>> > +typedef struct
>> > +{
>> > + long double a;
>> > + long double b;
>> > +} c __attribute__((aligned(32)));
>> > +extern double d;
>> > +void
>> > +bar (c f)
>> > +{
>> > + d = f.a;
>> > +}
>> > diff --git a/gcc/testsuite/gcc.target/i386/pr120839-2.c
>> > b/gcc/testsuite/gcc.target/i386/pr120839-2.c
>> > new file mode 100644
>> > index 00000000000..e5b711c966f
>> > --- /dev/null
>> > +++ b/gcc/testsuite/gcc.target/i386/pr120839-2.c
>> > @@ -0,0 +1,19 @@
>> > +/* { dg-do compile } */
>> > +/* { dg-options "-O2" } */
>> > +/* { dg-final { scan-assembler-not "and\[lq\]?\[\\t \]*\\$-32,\[\\t
>> > \]*%\[re\]?sp" } } */
>> > +
>> > +typedef struct
>> > +{
>> > + long double a;
>> > + long double b;
>> > +} c __attribute__((aligned(32)));
>> > +extern c x;
>> > +extern double d;
>> > +extern void bar (c);
>> > +void
>> > +foo (void)
>> > +{
>> > + x.a = d;
>> > + x.b = d;
>> > + bar (x);
>> > +}
