| Message ID | 3632939.GJXPN1kz7x@fomalhaut |
|---|---|
| State | New |
| Headers | show |
| Series | Fix PR C++/90448 | expand |
On Mon, Mar 8, 2021 at 6:19 PM Eric Botcazou <botcazou@adacore.com> wrote: > > Hi, > > this is a regression present on the mainline and 10 branch for architectures > that pass all structure types by reference, e.g. 32-bit PowerPC or SPARC. > > Jakub posted a detailed analysis in the audit trail and this boils down to > the RTL expander trying to take the address of a DECL whose RTX is a register. > > Bootstrapped/regtested on x86-64/Linux, PowerPC64/Linux and SPARC/Solaris, > OK for the mainline and 10 branch? The whole point of thunks is that they do not require things like copying ... is this case somehow IPA-SRA/CPed in an odd way? Otherwise it seems like the passed through reference was mishandled on the GIMPLE level? Honza? > > 2021-03-08 Eric Botcazou <ebotcazou@adacore.com> > > PR C++/90448 > * calls.c (initialize_argument_information): When the argument > is passed by reference, do not make a copy in a thunk only if > the argument is already in memory. Remove redundant test for > the case of callee copy. > > -- > Eric Botcazou
> The whole point of thunks is that they do not require things like copying > ... is this case somehow IPA-SRA/CPed in an odd way? Otherwise it seems > like the passed through reference was mishandled on the GIMPLE level? Maybe, but the change looks correct on its own and nobody has cared about the issue for months, so please let's not the best be the enemy of the good...
On Tue, Mar 9, 2021 at 10:18 AM Eric Botcazou <botcazou@adacore.com> wrote: > > > The whole point of thunks is that they do not require things like copying > > ... is this case somehow IPA-SRA/CPed in an odd way? Otherwise it seems > > like the passed through reference was mishandled on the GIMPLE level? > > Maybe, but the change looks correct on its own and nobody has cared about the > issue for months, so please let's not the best be the enemy of the good... Yes, the change looks correct - just the intent (for optimization purposes) is likely to still not have that copy ... So I'd say the patch is OK unless Honza violently objects but I think we want to investigate how we arrived at this unfortunate situation (it might be just an artifact of a strange ABI of course) Richard. > -- > Eric Botcazou > >
> On Mon, Mar 8, 2021 at 6:19 PM Eric Botcazou <botcazou@adacore.com> wrote: > > > > Hi, > > > > this is a regression present on the mainline and 10 branch for architectures > > that pass all structure types by reference, e.g. 32-bit PowerPC or SPARC. > > > > Jakub posted a detailed analysis in the audit trail and this boils down to > > the RTL expander trying to take the address of a DECL whose RTX is a register. > > > > Bootstrapped/regtested on x86-64/Linux, PowerPC64/Linux and SPARC/Solaris, > > OK for the mainline and 10 branch? > > The whole point of thunks is that they do not require things like copying ... is > this case somehow IPA-SRA/CPed in an odd way? Otherwise it seems like > the passed through reference was mishandled on the GIMPLE level? > > Honza? So this is the case where call_from_thunk is true but we do not produce tail call for some reason? For that it seems OK to me, just a missed optimization since copy should be unnecesary. So i guess the whole story is that we fail tailcall because we see parameter that needs copying because we lose track of its original memory location and only have value reloaded to register and we produce non-tail call in thunk... Honza > > > > > 2021-03-08 Eric Botcazou <ebotcazou@adacore.com> > > > > PR C++/90448 > > * calls.c (initialize_argument_information): When the argument > > is passed by reference, do not make a copy in a thunk only if > > the argument is already in memory. Remove redundant test for > > the case of callee copy. > > > > -- > > Eric Botcazou
diff --git a/gcc/calls.c b/gcc/calls.c index 1fea022ad8a..ff606204772 100644 --- a/gcc/calls.c +++ b/gcc/calls.c @@ -2388,19 +2388,17 @@ initialize_argument_information (int num_actuals ATTRIBUTE_UNUSED, function_arg_info arg (type, argpos < n_named_args); if (pass_by_reference (args_so_far_pnt, arg)) { - bool callee_copies; - tree base = NULL_TREE; - - callee_copies = reference_callee_copied (args_so_far_pnt, arg); - - /* If we're compiling a thunk, pass through invisible references - instead of making a copy. */ - if (call_from_thunk_p - || (callee_copies - && !TREE_ADDRESSABLE (type) - && (base = get_base_address (args[i].tree_value)) - && TREE_CODE (base) != SSA_NAME - && (!DECL_P (base) || MEM_P (DECL_RTL (base))))) + const bool callee_copies + = reference_callee_copied (args_so_far_pnt, arg); + tree base; + + /* If we're compiling a thunk, pass directly the address of an object + already in memory, instead of making a copy. Likewise if we want + to make the copy in the callee instead of the caller. */ + if ((call_from_thunk_p || callee_copies) + && (base = get_base_address (args[i].tree_value)) + && TREE_CODE (base) != SSA_NAME + && (!DECL_P (base) || MEM_P (DECL_RTL (base)))) { /* We may have turned the parameter value into an SSA name. Go back to the original parameter so we can take the