Message ID | 20210319094659.GF231854@tucnak |
---|---|
State | New |
Headers | show |
Series | aarch64: Don't emit -Wpsabi note when ABI was never affected [PR91710] | expand |
Jakub Jelinek <jakub@redhat.com> writes: > Hi! > > As the following testcase shows, we emit a -Wpsabi note about argument > passing change since GCC 9, but in reality the ABI didn't change. > The alignment is 8 bits in GCC < 9 and 32 bits in GCC >= 9 and > the aarch64_function_arg_alignment returns in that case: > return MIN (MAX (alignment, PARM_BOUNDARY), STACK_BOUNDARY); > so when both the old and new alignment are smaller or equal to PARM_BOUNDARY > (or both are larger than STACK_BOUNDARY, just in theory), even when the new > one is bigger, it doesn't change the argument passing. > > So, the following patch changes aarch64_function_arg_alignment to tell the > callers the exact old alignmentm so that they can test it if needed. > The other aarch64_function_arg_alignment callers either check the > alignment for equality against 16-byte alignment (when old alignment was > smaller than that and the new one is 16-byte, we want to emit -Wpsabi > in all the cases) or the va_arg case which I think is ok now too. > > Bootstrapped/regtested on aarch64-linux, ok for trunk? Looks good to me. Richard E knows this code better than I do though, so I think he should have the final say. He's currently on holiday but will be back next week. Thanks, Richard > > 2021-03-18 Jakub Jelinek <jakub@redhat.com> > > PR target/91710 > * config/aarch64/aarch64.c (aarch64_function_arg_alignment): Change > abi_break argument from bool * to unsigned *, store there the pre-GCC 9 > alignment. > (aarch64_layout_arg, aarch64_gimplify_va_arg_expr): Adjust callers. > (aarch64_function_arg_regno_p): Likewise. Only emit -Wpsabi note if > the old and new alignment after applying MIN/MAX to it is different. > > * gcc.target/aarch64/pr91710.c: New test. > > --- gcc/config/aarch64/aarch64.c.jj 2021-03-18 15:14:51.721425223 +0100 > +++ gcc/config/aarch64/aarch64.c 2021-03-18 16:35:04.437115447 +0100 > @@ -5938,9 +5938,9 @@ aarch64_vfp_is_call_candidate (cumulativ > > static unsigned int > aarch64_function_arg_alignment (machine_mode mode, const_tree type, > - bool *abi_break) > + unsigned int *abi_break) > { > - *abi_break = false; > + *abi_break = 0; > if (!type) > return GET_MODE_ALIGNMENT (mode); > > @@ -5982,7 +5982,7 @@ aarch64_function_arg_alignment (machine_ > > if (bitfield_alignment > alignment) > { > - *abi_break = true; > + *abi_break = alignment; > return bitfield_alignment; > } > > @@ -6004,7 +6004,7 @@ aarch64_layout_arg (cumulative_args_t pc > int ncrn, nvrn, nregs; > bool allocate_ncrn, allocate_nvrn; > HOST_WIDE_INT size; > - bool abi_break; > + unsigned int abi_break; > > /* We need to do this once per argument. */ > if (pcum->aapcs_arg_processed) > @@ -6322,14 +6322,19 @@ aarch64_function_arg_regno_p (unsigned r > static unsigned int > aarch64_function_arg_boundary (machine_mode mode, const_tree type) > { > - bool abi_break; > + unsigned int abi_break; > unsigned int alignment = aarch64_function_arg_alignment (mode, type, > &abi_break); > + alignment = MIN (MAX (alignment, PARM_BOUNDARY), STACK_BOUNDARY); > if (abi_break & warn_psabi) > - inform (input_location, "parameter passing for argument of type " > - "%qT changed in GCC 9.1", type); > + { > + abi_break = MIN (MAX (abi_break, PARM_BOUNDARY), STACK_BOUNDARY); > + if (alignment != abi_break) > + inform (input_location, "parameter passing for argument of type " > + "%qT changed in GCC 9.1", type); > + } > > - return MIN (MAX (alignment, PARM_BOUNDARY), STACK_BOUNDARY); > + return alignment; > } > > /* Implement TARGET_GET_RAW_RESULT_MODE and TARGET_GET_RAW_ARG_MODE. */ > @@ -16616,7 +16621,7 @@ aarch64_gimplify_va_arg_expr (tree valis > f_stack, NULL_TREE); > size = int_size_in_bytes (type); > > - bool abi_break; > + unsigned int abi_break; > align > = aarch64_function_arg_alignment (mode, type, &abi_break) / BITS_PER_UNIT; > > --- gcc/testsuite/gcc.target/aarch64/pr91710.c.jj 2021-03-18 16:42:33.529232710 +0100 > +++ gcc/testsuite/gcc.target/aarch64/pr91710.c 2021-03-18 16:42:07.271518121 +0100 > @@ -0,0 +1,16 @@ > +/* PR target/91710 */ > +/* { dg-do compile } */ > + > +struct S { unsigned int i:4; }; > + > +unsigned int test1(struct S s) { /* { dg-bogus "parameter passing for argument of type" } */ > + return s.i; > +} > + > +unsigned int test2(unsigned x, struct S s) { /* { dg-bogus "parameter passing for argument of type" } */ > + return x - s.i; > +} > + > +unsigned int test3(unsigned x, unsigned y, struct S s) { /* { dg-bogus "parameter passing for argument of type" } */ > + return x - y - s.i; > +} > > Jakub
On Fri, Mar 19, 2021 at 01:04:19PM +0000, Richard Sandiford via Gcc-patches wrote: > > As the following testcase shows, we emit a -Wpsabi note about argument > > passing change since GCC 9, but in reality the ABI didn't change. > > The alignment is 8 bits in GCC < 9 and 32 bits in GCC >= 9 and > > the aarch64_function_arg_alignment returns in that case: > > return MIN (MAX (alignment, PARM_BOUNDARY), STACK_BOUNDARY); > > so when both the old and new alignment are smaller or equal to PARM_BOUNDARY > > (or both are larger than STACK_BOUNDARY, just in theory), even when the new > > one is bigger, it doesn't change the argument passing. > > > > So, the following patch changes aarch64_function_arg_alignment to tell the > > callers the exact old alignmentm so that they can test it if needed. > > The other aarch64_function_arg_alignment callers either check the > > alignment for equality against 16-byte alignment (when old alignment was > > smaller than that and the new one is 16-byte, we want to emit -Wpsabi > > in all the cases) or the va_arg case which I think is ok now too. > > > > Bootstrapped/regtested on aarch64-linux, ok for trunk? > > Looks good to me. Richard E knows this code better than I do though, > so I think he should have the final say. He's currently on holiday > but will be back next week. I'd like to ping this patch. Thanks. > > 2021-03-18 Jakub Jelinek <jakub@redhat.com> > > > > PR target/91710 > > * config/aarch64/aarch64.c (aarch64_function_arg_alignment): Change > > abi_break argument from bool * to unsigned *, store there the pre-GCC 9 > > alignment. > > (aarch64_layout_arg, aarch64_gimplify_va_arg_expr): Adjust callers. > > (aarch64_function_arg_regno_p): Likewise. Only emit -Wpsabi note if > > the old and new alignment after applying MIN/MAX to it is different. > > > > * gcc.target/aarch64/pr91710.c: New test. > > > > --- gcc/config/aarch64/aarch64.c.jj 2021-03-18 15:14:51.721425223 +0100 > > +++ gcc/config/aarch64/aarch64.c 2021-03-18 16:35:04.437115447 +0100 > > @@ -5938,9 +5938,9 @@ aarch64_vfp_is_call_candidate (cumulativ > > > > static unsigned int > > aarch64_function_arg_alignment (machine_mode mode, const_tree type, > > - bool *abi_break) > > + unsigned int *abi_break) > > { > > - *abi_break = false; > > + *abi_break = 0; > > if (!type) > > return GET_MODE_ALIGNMENT (mode); > > > > @@ -5982,7 +5982,7 @@ aarch64_function_arg_alignment (machine_ > > > > if (bitfield_alignment > alignment) > > { > > - *abi_break = true; > > + *abi_break = alignment; > > return bitfield_alignment; > > } > > > > @@ -6004,7 +6004,7 @@ aarch64_layout_arg (cumulative_args_t pc > > int ncrn, nvrn, nregs; > > bool allocate_ncrn, allocate_nvrn; > > HOST_WIDE_INT size; > > - bool abi_break; > > + unsigned int abi_break; > > > > /* We need to do this once per argument. */ > > if (pcum->aapcs_arg_processed) > > @@ -6322,14 +6322,19 @@ aarch64_function_arg_regno_p (unsigned r > > static unsigned int > > aarch64_function_arg_boundary (machine_mode mode, const_tree type) > > { > > - bool abi_break; > > + unsigned int abi_break; > > unsigned int alignment = aarch64_function_arg_alignment (mode, type, > > &abi_break); > > + alignment = MIN (MAX (alignment, PARM_BOUNDARY), STACK_BOUNDARY); > > if (abi_break & warn_psabi) > > - inform (input_location, "parameter passing for argument of type " > > - "%qT changed in GCC 9.1", type); > > + { > > + abi_break = MIN (MAX (abi_break, PARM_BOUNDARY), STACK_BOUNDARY); > > + if (alignment != abi_break) > > + inform (input_location, "parameter passing for argument of type " > > + "%qT changed in GCC 9.1", type); > > + } > > > > - return MIN (MAX (alignment, PARM_BOUNDARY), STACK_BOUNDARY); > > + return alignment; > > } > > > > /* Implement TARGET_GET_RAW_RESULT_MODE and TARGET_GET_RAW_ARG_MODE. */ > > @@ -16616,7 +16621,7 @@ aarch64_gimplify_va_arg_expr (tree valis > > f_stack, NULL_TREE); > > size = int_size_in_bytes (type); > > > > - bool abi_break; > > + unsigned int abi_break; > > align > > = aarch64_function_arg_alignment (mode, type, &abi_break) / BITS_PER_UNIT; > > > > --- gcc/testsuite/gcc.target/aarch64/pr91710.c.jj 2021-03-18 16:42:33.529232710 +0100 > > +++ gcc/testsuite/gcc.target/aarch64/pr91710.c 2021-03-18 16:42:07.271518121 +0100 > > @@ -0,0 +1,16 @@ > > +/* PR target/91710 */ > > +/* { dg-do compile } */ > > + > > +struct S { unsigned int i:4; }; > > + > > +unsigned int test1(struct S s) { /* { dg-bogus "parameter passing for argument of type" } */ > > + return s.i; > > +} > > + > > +unsigned int test2(unsigned x, struct S s) { /* { dg-bogus "parameter passing for argument of type" } */ > > + return x - s.i; > > +} > > + > > +unsigned int test3(unsigned x, unsigned y, struct S s) { /* { dg-bogus "parameter passing for argument of type" } */ > > + return x - y - s.i; > > +} Jakub
On Mon, Mar 29, 2021 at 11:16:55AM +0200, Jakub Jelinek wrote: > > Looks good to me. Richard E knows this code better than I do though, > > so I think he should have the final say. He's currently on holiday > > but will be back next week. > > I'd like to ping this patch. Ping. > > > 2021-03-18 Jakub Jelinek <jakub@redhat.com> > > > > > > PR target/91710 > > > * config/aarch64/aarch64.c (aarch64_function_arg_alignment): Change > > > abi_break argument from bool * to unsigned *, store there the pre-GCC 9 > > > alignment. > > > (aarch64_layout_arg, aarch64_gimplify_va_arg_expr): Adjust callers. > > > (aarch64_function_arg_regno_p): Likewise. Only emit -Wpsabi note if > > > the old and new alignment after applying MIN/MAX to it is different. > > > > > > * gcc.target/aarch64/pr91710.c: New test. Thanks. Jakub
On Wed, Apr 07, 2021 at 03:53:26PM +0200, Jakub Jelinek via Gcc-patches wrote: > On Mon, Mar 29, 2021 at 11:16:55AM +0200, Jakub Jelinek wrote: > > > Looks good to me. Richard E knows this code better than I do though, > > > so I think he should have the final say. He's currently on holiday > > > but will be back next week. > > > > I'd like to ping this patch. > > Ping. Ping. > > > > 2021-03-18 Jakub Jelinek <jakub@redhat.com> > > > > > > > > PR target/91710 > > > > * config/aarch64/aarch64.c (aarch64_function_arg_alignment): Change > > > > abi_break argument from bool * to unsigned *, store there the pre-GCC 9 > > > > alignment. > > > > (aarch64_layout_arg, aarch64_gimplify_va_arg_expr): Adjust callers. > > > > (aarch64_function_arg_regno_p): Likewise. Only emit -Wpsabi note if > > > > the old and new alignment after applying MIN/MAX to it is different. > > > > > > > > * gcc.target/aarch64/pr91710.c: New test. Thanks Jakub
Jakub Jelinek via Gcc-patches <gcc-patches@gcc.gnu.org> writes: > On Wed, Apr 07, 2021 at 03:53:26PM +0200, Jakub Jelinek via Gcc-patches wrote: >> On Mon, Mar 29, 2021 at 11:16:55AM +0200, Jakub Jelinek wrote: >> > > Looks good to me. Richard E knows this code better than I do though, >> > > so I think he should have the final say. He's currently on holiday >> > > but will be back next week. >> > >> > I'd like to ping this patch. >> >> Ping. > > Ping. Let's go for it. I still think the patch looks good, and if a problem crops up then we still have time to fix it before the release. Applying it now seems better than applying it closer to the release. Thanks, Richard > >> > > > 2021-03-18 Jakub Jelinek <jakub@redhat.com> >> > > > >> > > > PR target/91710 >> > > > * config/aarch64/aarch64.c (aarch64_function_arg_alignment): Change >> > > > abi_break argument from bool * to unsigned *, store there the pre-GCC 9 >> > > > alignment. >> > > > (aarch64_layout_arg, aarch64_gimplify_va_arg_expr): Adjust callers. >> > > > (aarch64_function_arg_regno_p): Likewise. Only emit -Wpsabi note if >> > > > the old and new alignment after applying MIN/MAX to it is different. >> > > > >> > > > * gcc.target/aarch64/pr91710.c: New test. > > Thanks > > Jakub
--- gcc/config/aarch64/aarch64.c.jj 2021-03-18 15:14:51.721425223 +0100 +++ gcc/config/aarch64/aarch64.c 2021-03-18 16:35:04.437115447 +0100 @@ -5938,9 +5938,9 @@ aarch64_vfp_is_call_candidate (cumulativ static unsigned int aarch64_function_arg_alignment (machine_mode mode, const_tree type, - bool *abi_break) + unsigned int *abi_break) { - *abi_break = false; + *abi_break = 0; if (!type) return GET_MODE_ALIGNMENT (mode); @@ -5982,7 +5982,7 @@ aarch64_function_arg_alignment (machine_ if (bitfield_alignment > alignment) { - *abi_break = true; + *abi_break = alignment; return bitfield_alignment; } @@ -6004,7 +6004,7 @@ aarch64_layout_arg (cumulative_args_t pc int ncrn, nvrn, nregs; bool allocate_ncrn, allocate_nvrn; HOST_WIDE_INT size; - bool abi_break; + unsigned int abi_break; /* We need to do this once per argument. */ if (pcum->aapcs_arg_processed) @@ -6322,14 +6322,19 @@ aarch64_function_arg_regno_p (unsigned r static unsigned int aarch64_function_arg_boundary (machine_mode mode, const_tree type) { - bool abi_break; + unsigned int abi_break; unsigned int alignment = aarch64_function_arg_alignment (mode, type, &abi_break); + alignment = MIN (MAX (alignment, PARM_BOUNDARY), STACK_BOUNDARY); if (abi_break & warn_psabi) - inform (input_location, "parameter passing for argument of type " - "%qT changed in GCC 9.1", type); + { + abi_break = MIN (MAX (abi_break, PARM_BOUNDARY), STACK_BOUNDARY); + if (alignment != abi_break) + inform (input_location, "parameter passing for argument of type " + "%qT changed in GCC 9.1", type); + } - return MIN (MAX (alignment, PARM_BOUNDARY), STACK_BOUNDARY); + return alignment; } /* Implement TARGET_GET_RAW_RESULT_MODE and TARGET_GET_RAW_ARG_MODE. */ @@ -16616,7 +16621,7 @@ aarch64_gimplify_va_arg_expr (tree valis f_stack, NULL_TREE); size = int_size_in_bytes (type); - bool abi_break; + unsigned int abi_break; align = aarch64_function_arg_alignment (mode, type, &abi_break) / BITS_PER_UNIT; --- gcc/testsuite/gcc.target/aarch64/pr91710.c.jj 2021-03-18 16:42:33.529232710 +0100 +++ gcc/testsuite/gcc.target/aarch64/pr91710.c 2021-03-18 16:42:07.271518121 +0100 @@ -0,0 +1,16 @@ +/* PR target/91710 */ +/* { dg-do compile } */ + +struct S { unsigned int i:4; }; + +unsigned int test1(struct S s) { /* { dg-bogus "parameter passing for argument of type" } */ + return s.i; +} + +unsigned int test2(unsigned x, struct S s) { /* { dg-bogus "parameter passing for argument of type" } */ + return x - s.i; +} + +unsigned int test3(unsigned x, unsigned y, struct S s) { /* { dg-bogus "parameter passing for argument of type" } */ + return x - y - s.i; +}