Richard Biener <[email protected]> writes: > The following fixes a confusion seein in the x86 backend by > assign_parm_adjust_stack_rtl failing to trigger a local stack copy > for an incoming stack parameter that is not aligned according to > its type. The condition was introduced in r0-64961-gbfc45551d5ace4 > but there is the MEM_ALIGN (stack_parm) < PREFERRED_STACK_BOUNDARY > condition not triggering for the case in question where both > MEM_ALIGN and PREFERRED_STACK_BOUNDARY are 128. I cannot really > make sense of the condition - taking MEM_ALIGN (stack_parm) > as the ABI mandated alignment of the argument stack slot means > that is a lower bound on the incoming stack boundary. But > PREFERRED_STACK_BOUNDARY is not the actual boundary and relating > it to a minimum known boundary to somehow infer known bigger > alignment of the incoming parameter looks odd.
I have no memory of this patch, but my reading is that PREFERRED_STACK_BOUNDARY is being used as the maximum attainable alignment. In other words, there is no point discarding the old underaligned stack slot if the new one wouldn't guarantee a greater alignment. According to this assumption, GCC's stack allocation code wouldn't preserve an alignment greater than PREFERRED_STACK_BOUNDARY even if the incoming stack pointer happened to be more aligned. However, this predates the dynamic stack realignment code (r0-88648-g2e3f842fe69ce4), so perhaps we should skip the PREFERRED_STACK_BOUNDARY check on targets that use that? E.g. on aarch64, if a type claims to have 32-byte alignment, GCC won't try to provide that alignment for arguments passed on the stack, whereas x86 will. https://godbolt.org/z/eWjfcoPbb is a not very good example. (Not very good because the parameter is passed by indirect reference on aarch64. I think the point still stands though.) Changing < to <= doesn't look right for other targets though. It's likely to lead to pointless copying. Thanks, Richard > > As minimal surgery at this point I'm not removing the condition > but make it include PREFERRED_STACK_BOUNDARY to make the copy > for the testcase in question. Correctness-wise I believe the > condition should be elided. > > Alan - you added this check, can you possibly guess what it was > supposed to guard against? Richard H., you approved the change in the > end, what's the reason for the PREFERRED_STACK_BOUNDARY guard? Richard > S., you became involved in the discussion at the end as well. > > Bootstrapped and tested on x86_64-unknown-linux-gnu. > > OK for trunk? > > Thanks, > Richard. > > PR middle-end/120839 > * function.cc (assign_parm_adjust_stack_rtl): Adjust > alignment check forcing a local copy. > > * gcc.dg/torture/pr120839.c: New testcase. > --- > gcc/function.cc | 2 +- > gcc/testsuite/gcc.dg/torture/pr120839.c | 7 +++++++ > 2 files changed, 8 insertions(+), 1 deletion(-) > create mode 100644 gcc/testsuite/gcc.dg/torture/pr120839.c > > diff --git a/gcc/function.cc b/gcc/function.cc > index bba05f3380d..e93d12785a5 100644 > --- a/gcc/function.cc > +++ b/gcc/function.cc > @@ -2840,7 +2840,7 @@ assign_parm_adjust_stack_rtl (struct > assign_parm_data_one *data) > MEM_ALIGN (stack_parm)))) > || (data->nominal_type > && TYPE_ALIGN (data->nominal_type) > MEM_ALIGN (stack_parm) > - && MEM_ALIGN (stack_parm) < PREFERRED_STACK_BOUNDARY))) > + && MEM_ALIGN (stack_parm) <= PREFERRED_STACK_BOUNDARY))) > stack_parm = NULL; > > /* If parm was passed in memory, and we need to convert it on entry, > diff --git a/gcc/testsuite/gcc.dg/torture/pr120839.c > b/gcc/testsuite/gcc.dg/torture/pr120839.c > new file mode 100644 > index 00000000000..158e800649f > --- /dev/null > +++ b/gcc/testsuite/gcc.dg/torture/pr120839.c > @@ -0,0 +1,7 @@ > +/* { dg-do compile } */ > + > +typedef struct { > + long double a, b; > +} c __attribute__((aligned(32))); > +double d; > +void e(c f) { d = f.a; }
