[1/2] symtab: also change RTL decl name

  We were changing the ASSEMBLER_NAME of the function decl
but not the name in DECL_RTL which is used as the function name
fnname in rest_of_handle_final(). This led to using the old, wrong name
for the attribute target default function when using target_clones.

Bootstrapped and regtested cleanly on x86_64-unknown-linux
for c,c++,fortran,lto.
Ok for trunk?

gcc/ChangeLog:

	* symtab.cc: Remove stray comment.
	(symbol_table::change_decl_assembler_name): Also update the
	name in DECL_RTL.

Cc: Jan Hubicka <hubicka@ucw.cz>
---
 gcc/symtab.cc | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
  

Hi Honza, Ping.
Regtests cleanly for c,fortran,c++,ada,d,go,lto,objc,obj-c++
Ok?
I'd need this for attribute target_clones for the Fortran FE.
thanks,

On Wed,  9 Nov 2022 20:02:24 +0100
Bernhard Reutner-Fischer <rep.dot.nop@gmail.com> wrote:

> We were changing the ASSEMBLER_NAME of the function decl
> but not the name in DECL_RTL which is used as the function name
> fnname in rest_of_handle_final(). This led to using the old, wrong name
> for the attribute target default function when using target_clones.
> 
> Bootstrapped and regtested cleanly on x86_64-unknown-linux
> for c,c++,fortran,lto.
> Ok for trunk?
> 
> gcc/ChangeLog:
> 
> 	* symtab.cc: Remove stray comment.
> 	(symbol_table::change_decl_assembler_name): Also update the
> 	name in DECL_RTL.
> 
> Cc: Jan Hubicka <hubicka@ucw.cz>
> ---
>  gcc/symtab.cc | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/gcc/symtab.cc b/gcc/symtab.cc
> index f2d96c0268b..2e20bf5fefc 100644
> --- a/gcc/symtab.cc
> +++ b/gcc/symtab.cc
> @@ -154,8 +154,6 @@ symbol_table::decl_assembler_name_equal (tree decl, const_tree asmname)
>  }
>  
>  
> -/* Returns nonzero if P1 and P2 are equal.  */
> -
>  /* Insert NODE to assembler name hash.  */
>  
>  void
> @@ -303,6 +301,10 @@ symbol_table::change_decl_assembler_name (tree decl, tree name)
>  	warning (0, "%qD renamed after being referenced in assembly", decl);
>  
>        SET_DECL_ASSEMBLER_NAME (decl, name);
> +      /* Set the new name in rtl.  */
> +      if (DECL_RTL_SET_P (decl))
> +	XSTR (XEXP (DECL_RTL (decl), 0), 0) = IDENTIFIER_POINTER (name);
> +
>        if (alias)
>  	{
>  	  IDENTIFIER_TRANSPARENT_ALIAS (name) = 1;
  

Hello,

Le 17/11/2022 à 09:02, Bernhard Reutner-Fischer via Fortran a écrit :
> Hi Honza, Ping.
> Regtests cleanly for c,fortran,c++,ada,d,go,lto,objc,obj-c++
> Ok?
> I'd need this for attribute target_clones for the Fortran FE.
> thanks,
> 
> On Wed,  9 Nov 2022 20:02:24 +0100
> Bernhard Reutner-Fischer <rep.dot.nop@gmail.com> wrote:
> 
>> We were changing the ASSEMBLER_NAME of the function decl
>> but not the name in DECL_RTL which is used as the function name
>> fnname in rest_of_handle_final(). This led to using the old, wrong name
>> for the attribute target default function when using target_clones.
>>
>> Bootstrapped and regtested cleanly on x86_64-unknown-linux
>> for c,c++,fortran,lto.
>> Ok for trunk?
>>
>> gcc/ChangeLog:
>>
>> 	* symtab.cc: Remove stray comment.
>> 	(symbol_table::change_decl_assembler_name): Also update the
>> 	name in DECL_RTL.
>>
(patch stripped)

Is there a PR about this?  Or a testcase exhibiting the problem at least?
  

> Hi Honza, Ping.
> Regtests cleanly for c,fortran,c++,ada,d,go,lto,objc,obj-c++
> Ok?
> I'd need this for attribute target_clones for the Fortran FE.
Sorry for delay here.
> >  void
> > @@ -303,6 +301,10 @@ symbol_table::change_decl_assembler_name (tree decl, tree name)
> >  	warning (0, "%qD renamed after being referenced in assembly", decl);
> >  
> >        SET_DECL_ASSEMBLER_NAME (decl, name);
> > +      /* Set the new name in rtl.  */
> > +      if (DECL_RTL_SET_P (decl))
> > +	XSTR (XEXP (DECL_RTL (decl), 0), 0) = IDENTIFIER_POINTER (name);

I am not quite sure how safe this is.  We generally produce DECL_RTL
when we produce assembly file.  So if DECL_RTL is set then we probably
already output the original function name and it is too late to change
it.

Also RTL is shared so changing it in-place is going to rewrite all the
existing RTL expressions using it.

Why the DECL_RTL is produced for function you want to rename?
Honza
> > +
> >        if (alias)
> >  	{
> >  	  IDENTIFIER_TRANSPARENT_ALIAS (name) = 1;
>
  

On Mon, 21 Nov 2022 20:02:49 +0100
Jan Hubicka <hubicka@ucw.cz> wrote:

> > Hi Honza, Ping.
> > Regtests cleanly for c,fortran,c++,ada,d,go,lto,objc,obj-c++
> > Ok?
> > I'd need this for attribute target_clones for the Fortran FE.  
> Sorry for delay here.
> > >  void
> > > @@ -303,6 +301,10 @@ symbol_table::change_decl_assembler_name (tree decl, tree name)
> > >  	warning (0, "%qD renamed after being referenced in assembly", decl);
> > >  
> > >        SET_DECL_ASSEMBLER_NAME (decl, name);
> > > +      /* Set the new name in rtl.  */
> > > +      if (DECL_RTL_SET_P (decl))
> > > +	XSTR (XEXP (DECL_RTL (decl), 0), 0) = IDENTIFIER_POINTER (name);  
> 
> I am not quite sure how safe this is.  We generally produce DECL_RTL
> when we produce assembly file.  So if DECL_RTL is set then we probably
> already output the original function name and it is too late to change
> it.

AFAICS we make_decl_rtl in the fortran FE in trans_function_start.

> 
> Also RTL is shared so changing it in-place is going to rewrite all the
> existing RTL expressions using it.
> 
> Why the DECL_RTL is produced for function you want to rename?

I think the fortran FE sets it quite early when lowering a function.
Later, when the ME creates the target_clones, it wants to rename the
initial function to initial_fun.default for the default target.
That's where the change_decl_assembler_name is called (only on the
decl).
But nobody changes the RTL name, so the ifunc (which should be the
initial, unchanged name) is properly emitted but
assemble_start_function uses the same, unchanged, initial fnname it
later obtains by get_fnname_from_decl which fetches the (wrong) initial
name where it should use the .default target name.
See
https://gcc.gnu.org/pipermail/gcc-patches/2022-November/605081.html

I'm open to other suggestions to make this work in a different way, of
course. Maybe we're missing some magic somewhere that might share the
name between the fndecl and the RTL XSTR so the RTL is magically
updated by that single SET_ECL_ASSEMBLER_NAME in
change_decl_assembler_name? But i didn't quite see where that'd be?

thanks,

> Honza
> > > +
> > >        if (alias)
> > >  	{
> > >  	  IDENTIFIER_TRANSPARENT_ALIAS (name) = 1;  
> >
  

> On Mon, 21 Nov 2022 20:02:49 +0100
> Jan Hubicka <hubicka@ucw.cz> wrote:
> 
> > > Hi Honza, Ping.
> > > Regtests cleanly for c,fortran,c++,ada,d,go,lto,objc,obj-c++
> > > Ok?
> > > I'd need this for attribute target_clones for the Fortran FE.  
> > Sorry for delay here.
> > > >  void
> > > > @@ -303,6 +301,10 @@ symbol_table::change_decl_assembler_name (tree decl, tree name)
> > > >  	warning (0, "%qD renamed after being referenced in assembly", decl);
> > > >  
> > > >        SET_DECL_ASSEMBLER_NAME (decl, name);
> > > > +      /* Set the new name in rtl.  */
> > > > +      if (DECL_RTL_SET_P (decl))
> > > > +	XSTR (XEXP (DECL_RTL (decl), 0), 0) = IDENTIFIER_POINTER (name);  
> > 
> > I am not quite sure how safe this is.  We generally produce DECL_RTL
> > when we produce assembly file.  So if DECL_RTL is set then we probably
> > already output the original function name and it is too late to change
> > it.
> 
> AFAICS we make_decl_rtl in the fortran FE in trans_function_start.

I see, it may be a relic of something that is no longer necessary.  Can
you see why one needs DECL_RTL so early?
> 
> > 
> > Also RTL is shared so changing it in-place is going to rewrite all the
> > existing RTL expressions using it.
> > 
> > Why the DECL_RTL is produced for function you want to rename?
> 
> I think the fortran FE sets it quite early when lowering a function.
> Later, when the ME creates the target_clones, it wants to rename the
> initial function to initial_fun.default for the default target.
> That's where the change_decl_assembler_name is called (only on the
> decl).
> But nobody changes the RTL name, so the ifunc (which should be the
> initial, unchanged name) is properly emitted but
> assemble_start_function uses the same, unchanged, initial fnname it
> later obtains by get_fnname_from_decl which fetches the (wrong) initial
> name where it should use the .default target name.
> See
> https://gcc.gnu.org/pipermail/gcc-patches/2022-November/605081.html
> 
> I'm open to other suggestions to make this work in a different way, of
> course. Maybe we're missing some magic somewhere that might share the
> name between the fndecl and the RTL XSTR so the RTL is magically
> updated by that single SET_ECL_ASSEMBLER_NAME in
> change_decl_assembler_name? But i didn't quite see where that'd be?

I think we should start by understanding why Fortran FE produces
DECL_RTL early.  It was written before symbol table code emerged, it may
be simply an oversight I made while converting FE to symbol table.

Honza
> 
> thanks,
> 
> > Honza
> > > > +
> > > >        if (alias)
> > > >  	{
> > > >  	  IDENTIFIER_TRANSPARENT_ALIAS (name) = 1;  
> > >   
>
  

On Tue, 22 Nov 2022 12:54:01 +0100
Jan Hubicka <hubicka@ucw.cz> wrote:

> > > I am not quite sure how safe this is.  We generally produce DECL_RTL
> > > when we produce assembly file.  So if DECL_RTL is set then we probably
> > > already output the original function name and it is too late to change
> > > it.  
> > 
> > AFAICS we make_decl_rtl in the fortran FE in trans_function_start.  
> 
> I see, it may be a relic of something that is no longer necessary.  Can
> you see why one needs DECL_RTL so early?

No.

I think this is a ward, isn't it.

diff --git a/gcc/fortran/trans-decl.cc b/gcc/fortran/trans-decl.cc
index ff64588b9a8..a801e66fb11 100644
--- a/gcc/fortran/trans-decl.cc
+++ b/gcc/fortran/trans-decl.cc
@@ -2922,7 +2922,7 @@ trans_function_start (gfc_symbol * sym)
     }
 
   /* Create RTL for function definition.  */
-  make_decl_rtl (fndecl);
+  //make_decl_rtl (fndecl);
 
   allocate_struct_function (fndecl, false);

But we have that alot, with varous workarounds near
lhd_set_decl_assembler_name.
gcc.gnu.org/pr94223 comes to mind, but that was counters.
But i've seen in C++ too that there are GC dangles like here.

5f3682ffcef162363b783eb9ee702debff489fa8 a.k.a
https://gcc.gnu.org/legacy-ml/gcc-patches/2017-11/msg01340.html

ah lhd_overwrite_decl_assembler_name.

So.. why do we have that, again?
Per default it doesn't look much if there are clones or an ifunc
dispatcher, does it.