On 01/03/2024 04:38, Alexandre Oliva wrote: > Hello, Matthew, > > Thanks for the review.
For closure, Jakub has just pushed a patch to the generic code, so I don't think we need this now. R. > > On Feb 26, 2024, Matthew Malcomson <matthew.malcom...@arm.com> wrote: > >> I think you're right that the AAPCS32 requires all arguments to be passed in >> registers for this testcase. >> (Nit on the commit-message: It says that your reading of the AAPCS32 >> suggests >> that the *caller* is correct -- I believe based on the change you >> suggested you >> meant *callee* is correct in expecting arguments in registers.) > > Ugh, yeah, sorry about the typo. > >> The approach you suggest looks OK to me -- I do notice that it doesn't >> fix the >> legacy ABI's of `atpcs` and `apcs` and guess it would be nicer to have them >> working at the same time though would defer to maintainers on how >> important that >> is. >> (For the benefit of others reading) I don't believe there is any ABI concern >> with this since it's fixing something that is currently not working at >> all and >> only applies to c23 (so a change shouldn't have too much of an impact). > >> You mention you chose to make the change in the arm backend rather >> than general >> code due to hesitancy to change the generic ABI-affecting code. That makes >> sense to me, certainly at this late stage in the development cycle. > > *nod* I wrote the patch in the following context: I hit the problem on > the very first toolchain I started transitioning to gcc-13. I couldn't > really fathom the notion that this breakage could have survived an > entire release cycle if it affected many targets, and sort of held on to > an assumption that the abi used by our arm-eabi toolchain had to be an > uncommon one. > > All of this hypothesizing falls apart by the now apparent knowledge that > the test is faling elsewhere as well, even on other ARM ABIs, it just > hadn't been addressed yet. I'm glad we're getting there :-) > >> From a quick check on c23-stdarg-4.c it does look like the below >> change ends up >> with the same codegen as your patch (except in the case of those >> legacy ABI's, >> where the below does make the caller and callee ABI match AFAICT): > >> ``` >> diff --git a/gcc/calls.cc b/gcc/calls.cc >> index 01f44734743..0b302f633ed 100644 >> --- a/gcc/calls.cc >> +++ b/gcc/calls.cc >> @@ -2970,14 +2970,15 @@ expand_call (tree exp, rtx target, int ignore) >> we do not have any reliable way to pass unnamed args in >> registers, so we must force them into memory. */ > >> - if (type_arg_types != 0 >> + if ((type_arg_types != 0 || TYPE_NO_NAMED_ARGS_STDARG_P (funtype)) >> && targetm.calls.strict_argument_naming (args_so_far)) >> ; >> else if (type_arg_types != 0 >> && ! targetm.calls.pretend_outgoing_varargs_named >> (args_so_far)) >> /* Don't include the last named arg. */ >> --n_named_args; >> - else if (TYPE_NO_NAMED_ARGS_STDARG_P (funtype)) >> + else if (TYPE_NO_NAMED_ARGS_STDARG_P (funtype) >> + && ! targetm.calls.pretend_outgoing_varargs_named (args_so_far)) >> n_named_args = 0; >> else >> /* Treat all args as named. */ >> ``` > >> Do you agree that this makes sense (i.e. is there something I'm >> completely missing)? > > Yeah, your argument is quite convincing, and the target knobs are indeed > in line with the change you suggest, whereas the current code seems to > deviate from them. > > With my ABI designer hat on, however, I see that there's room for ABIs > to make decisions about 0-args stdargs that go differently from stdargs > with leading named args, from prototyped functions, and even from > prototypeless functions, and we might end up needing more knobs to deal > with such custom decisions. We can cross that bridge if/when we get to > it, though. > >> (lm32 mcore msp430 gcn cris fr30 frv h8300 arm v850 rx pru) > > Interesting that ppc64le is not on your list. There's PR107453 about > that, and another thread is discussing a fix for it that is somewhat > different from what you propose (presumably because the way the problem > manifests on ppc64le is different), but it also tweaks expand_call. > > I'll copy you when following up there. >
