[patch] Fix PR C++/90448

[patch] Fix PR C++/90448

[Eric Botcazou]

[-- Attachment #1: Type: text/plain, Size: 724 bytes --]

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?


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

[-- Attachment #2: pr90448.diff --]
[-- Type: text/x-patch, Size: 1427 bytes --]

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

Re: [patch] Fix PR C++/90448

[Richard Biener]

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

Re: [patch] Fix PR C++/90448

[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...

-- 
Eric Botcazou

Re: [patch] Fix PR C++/90448

[Richard Biener]

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
>
>

Re: [patch] Fix PR C++/90448

[Jan Hubicka]

> 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