RFH: attr target_clones default assembler name ignored?

RFH: attr target_clones default assembler name ignored?

[Bernhard Reutner-Fischer]

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

Hi!

I encounter a problem/pilot error when using the target_clones
attribute.

The symptom is that the default clone is not renamed in the output and
thus conflicts with the (proper) global name which is used for the
ifunc:

$ nl -ba < /tmp/cc3jBX3x.s | grep sub1
    12		.type	__m_MOD_sub1, @function
    13	__m_MOD_sub1:
    35		.size	__m_MOD_sub1, .-__m_MOD_sub1
...
    87	__m_MOD_sub1.resolver:
    95		movl	$__m_MOD_sub1.avx, %eax
   104		movl	$__m_MOD_sub1, %eax
   105		movl	$__m_MOD_sub1.sse, %edx
   110		.size	__m_MOD_sub1.resolver, .-__m_MOD_sub1.resolver
   111		.globl	__m_MOD_sub1
   112		.type	__m_MOD_sub1, @gnu_indirect_function
   113		.set	__m_MOD_sub1,__m_MOD_sub1.resolver

I think that line 13 and 104 should talk about __m_MOD_sub1.default.

AFAICT the target_clones attr is built well.
gcc/multiple_target.cc:expand_target_clones() adds the required
attr "target" "default"
and properly does:
(gdb) p old_name
$6 = 0x7ffff6dfc090 "__m_MOD_sub1"
(gdb) call debug_tree ( DECL_ASSEMBLER_NAME (decl) )
 <identifier_node 0x7ffff6dfd640 __m_MOD_sub1>
(gdb) n
301		  && DECL_RTL_SET_P (decl))
(gdb) n
300	      if (TREE_SYMBOL_REFERENCED (DECL_ASSEMBLER_NAME (decl))
(gdb) n
304	      SET_DECL_ASSEMBLER_NAME (decl, name);
(gdb) n
305	      if (alias)
(gdb) call debug_tree ( DECL_ASSEMBLER_NAME (decl) )
 <identifier_node 0x7ffff6a0b758 __m_MOD_sub1.default>

So if that would have been used for real, all would be fine i think.

But still, that name is somehow not used, see fnname:
#0  assemble_start_function (decl=<function_decl 0x7ffff6df9500 sub1>, fnname=fnname@entry=0x7ffff6dfc090 "__m_MOD_sub1") at ../../../src/gcc-13.mine/gcc/varasm.cc:1979
#1  0x0000000000bec182 in rest_of_handle_final () at ../../../src/gcc-13.mine/gcc/final.cc:4238
#2  (anonymous namespace)::pass_final::execute (this=<optimized out>) at ../../../src/gcc-13.mine/gcc/final.cc:4320
#3  0x0000000000e9da3b in execute_one_pass (pass=<opt_pass* 0x2dbc900 "final"(343)>) at ../../../src/gcc-13.mine/gcc/passes.cc:2644
#4  0x0000000000e9e300 in execute_pass_list_1 (pass=<opt_pass* 0x2dbc900 "final"(343)>) at ../../../src/gcc-13.mine/gcc/passes.cc:2744
#5  0x0000000000e9e312 in execute_pass_list_1 (pass=<opt_pass* 0x2dbc3b0 "*all-late_compilation"(-1)>) at ../../../src/gcc-13.mine/gcc/passes.cc:2745
#6  0x0000000000e9e312 in execute_pass_list_1 (pass=<opt_pass* 0x2dba710 "*rest_of_compilation"(-1)>) at ../../../src/gcc-13.mine/gcc/passes.cc:2745
#7  0x0000000000e9e339 in execute_pass_list (fn=0x7ffff6dfe000, pass=<optimized out>) at ../../../src/gcc-13.mine/gcc/passes.cc:2755
#8  0x0000000000aef806 in cgraph_node::expand (this=<cgraph_node * const 0x7ffff6c19220 "sub1"/0>) at ../../../src/gcc-13.mine/gcc/context.h:48
#9  cgraph_node::expand (this=<cgraph_node * const 0x7ffff6c19220 "sub1"/0>) at ../../../src/gcc-13.mine/gcc/cgraphunit.cc:1787

I'm attaching the testcase, to be compiled with:
$ ./gfortran -B. -o /tmp/out.o -c /scratch/src/gcc-13.mine/gcc/testsuite/gfortran.dg/attr_target_clones-1.f90 -O1 -fdump-tree-optimized
/tmp/ccQU3Vkf.s: Assembler messages:
/tmp/ccQU3Vkf.s:113: Error: symbol `__m_MOD_sub1' is already defined
respectively
$ ./gfortran -B. -o /tmp/out.s -S /scratch/src/gcc-13.mine/gcc/testsuite/gfortran.dg/attr_target_clones-1.f90 -O1 -fdump-tree-optimized
and a git diff, which is not to be mistaken for a patch.

I'd be grateful for help to understand why/who chooses to pick an unpleasant
name, and, primarily, how to avoid that problem.

Thoughts or hints?
TIA!
PS: Should i have rather asked on gcc-help?

[-- Attachment #2: attr_target_clones-1.f90 --]
[-- Type: text/plain, Size: 791 bytes --]

! { dg-do compile }
! { dg-options "-O1 -fdump-tree-optimized" }
!
! Test __attribute__ ((target_clones ("foo", "bar")))
!
module m
  implicit none
!!GCC$ ATTRIBUTES target_clones("avx2", "sse2","default") :: sub2
contains
  subroutine sub1()
!GCC$ ATTRIBUTES target_clones("avx", "sse","default") :: sub1
    print *, 4321
  end
!  subroutine sub2()
!    print *, 2345
!  end
end module m
! { dg-final { scan-tree-dump-times {void * __m_MOD_sub1.resolver ()} "optimized" 1 } }
! { dg-final { scan-tree-dump-times {void __m_MOD_sub1.avx ()} "optimized" 1 } }
! { dg-final { scan-tree-dump-times {void __m_MOD_sub1.sse ()} "optimized" 1 } }
! { dg-final { scan-tree-dump-times {void __m_MOD_sub1.default ()} "optimized" 1 } }
! { dg-final { scan-tree-dump-not {void sub1 ()} "optimized" } }


[-- Attachment #3: fwd-fortran-attributes.00c.patch --]
[-- Type: text/plain, Size: 6993 bytes --]

diff --git a/gcc/fortran/decl.cc b/gcc/fortran/decl.cc
index 0f9b2ced4c2..ca70b79db57 100644
--- a/gcc/fortran/decl.cc
+++ b/gcc/fortran/decl.cc
@@ -23,6 +23,7 @@ along with GCC; see the file COPYING3.  If not see
 #include "coretypes.h"
 #include "options.h"
 #include "tree.h"
+#include "fold-const.h"
 #include "gfortran.h"
 #include "stringpool.h"
 #include "match.h"
@@ -35,7 +36,7 @@ along with GCC; see the file COPYING3.  If not see
 #define gfc_get_data_variable() XCNEW (gfc_data_variable)
 #define gfc_get_data_value() XCNEW (gfc_data_value)
 #define gfc_get_data() XCNEW (gfc_data)
-
+#pragma GCC optimize("O0")
 
 static bool set_binding_label (const char **, const char *, int);
 
@@ -11709,6 +11710,92 @@ gfc_match_final_decl (void)
   return MATCH_YES;
 }
 
+/* Internal helper to parse attribute argument list.
+   If REQUIRE_STRING is true, then require a string.
+   If ALLOW_MULTIPLE is true, allow more than one arg.
+   If multiple arguments are passed, require braces around them.
+   Returns a tree_list of arguments or NULL_TREE.  */
+static tree
+gfc_match_gcc_attribute_args (bool require_string, bool allow_multiple)
+{
+  vec<tree, va_gc> *expr_list;
+  tree attr_args = NULL_TREE, attr_arg;
+  char name[GFC_MAX_SYMBOL_LEN + 1];
+  unsigned pos = 0;
+  gfc_char_t c;
+
+  gfc_gobble_whitespace ();
+
+  if (allow_multiple && gfc_match_char ('(') != MATCH_YES)
+    {
+      gfc_error ("expected '(' at %C");
+      return NULL_TREE;
+    }
+
+  if (require_string)
+    { /* XXX: Rephrase this in a sane, understandable manner..  */
+      do {
+	if (pos)
+	  {
+	    if (!allow_multiple)
+	      {
+		gfc_error ("surplus argument at %C");
+		return NULL_TREE;
+	      }
+	    gfc_next_ascii_char (); /* Consume the comma.  */
+	  }
+	pos = 0;
+	gfc_gobble_whitespace ();
+	unsigned char num_quotes = 0;
+	do {
+	  /* This should be done more efficiently.  wide_strchr nextc ?  */
+	  c = gfc_next_char_literal (NONSTRING);
+	  if (c == '"')
+	    num_quotes++;
+	  name[pos++] = c;
+	  if (pos >= GFC_MAX_SYMBOL_LEN)
+	    {
+	      gfc_error ("attribute argument truncated at %C");
+	      return NULL_TREE;
+	    }
+	} while (num_quotes % 2 && gfc_match_eos () != MATCH_YES);
+	if (pos < 1)
+	  {
+	    gfc_error ("expected argument at %C");
+	    return NULL_TREE;
+	  }
+	if (num_quotes != 2)
+	  {
+	    gfc_error ("invalid string literal at %C");
+	    return NULL_TREE;
+	  }
+	name[pos] = '\0'; /* Redundant wrt build_string.  */
+	tree str;
+	if (name[0] == '"')
+	  str = build_string (pos -= 2, name + 1); /* Trim quotes */
+	else
+	  str = build_string (pos, name);
+	/* Compare with c-family/c-common.cc: fix_string_type.  */
+	tree i_type = build_index_type (size_int (pos));
+	tree a_type = build_array_type (char_type_node, i_type);
+	TREE_TYPE (str) = a_type;
+	TREE_READONLY (str) = 1;
+	TREE_STATIC (str) = 1;
+	attr_arg = build_tree_list (NULL_TREE, str);
+	attr_args = chainon (attr_args, attr_arg);
+
+	gfc_gobble_whitespace ();
+      } while (gfc_peek_ascii_char () == ',');
+    }
+
+  if (allow_multiple && gfc_match_char (')') != MATCH_YES)
+    {
+      gfc_error ("expected ')' at %C");
+      return NULL_TREE;
+    }
+
+  return attr_args;
+}
 
 const ext_attr_t ext_attr_list[] = {
   { "dllimport",    EXT_ATTR_DLLIMPORT,    "dllimport" },
@@ -11718,6 +11805,7 @@ const ext_attr_t ext_attr_list[] = {
   { "fastcall",     EXT_ATTR_FASTCALL,     "fastcall"  },
   { "no_arg_check", EXT_ATTR_NO_ARG_CHECK, NULL        },
   { "deprecated",   EXT_ATTR_DEPRECATED,   NULL	       },
+  { "target_clones",EXT_ATTR_TARGET_CLONES,NULL	       },
   { NULL,           EXT_ATTR_LAST,         NULL        }
 };
 
@@ -11743,6 +11831,7 @@ gfc_match_gcc_attributes (void)
   unsigned id;
   gfc_symbol *sym;
   match m;
+  tree attr_args = NULL_TREE;
 
   gfc_clear_attr (&attr);
   for(;;)
@@ -11761,6 +11850,15 @@ gfc_match_gcc_attributes (void)
 	  gfc_error ("Unknown attribute in !GCC$ ATTRIBUTES statement at %C");
 	  return MATCH_ERROR;
 	}
+      else if (id == EXT_ATTR_TARGET_CLONES)
+	{
+	  attr_args
+	    = gfc_match_gcc_attribute_args(true, true);
+	  if (attr_args != NULL_TREE)
+	    attr.ext_attr_args
+	      = chainon (attr.ext_attr_args,
+			 build_tree_list (get_identifier (name), attr_args));
+	}
 
       if (!gfc_add_ext_attribute (&attr, (ext_attr_id_t)id, &gfc_current_locus))
 	return MATCH_ERROR;
@@ -11793,6 +11891,8 @@ gfc_match_gcc_attributes (void)
 	return MATCH_ERROR;
 
       sym->attr.ext_attr |= attr.ext_attr;
+      sym->attr.ext_attr_args
+	= chainon (sym->attr.ext_attr_args, attr.ext_attr_args);
 
       if (gfc_match_eos () == MATCH_YES)
 	break;
diff --git a/gcc/fortran/f95-lang.cc b/gcc/fortran/f95-lang.cc
index a6750bea787..7154568aec5 100644
--- a/gcc/fortran/f95-lang.cc
+++ b/gcc/fortran/f95-lang.cc
@@ -97,6 +97,10 @@ static const struct attribute_spec gfc_attribute_table[] =
     gfc_handle_omp_declare_target_attribute, NULL },
   { "oacc function", 0, -1, true,  false, false, false,
     gfc_handle_omp_declare_target_attribute, NULL },
+  { "target",                 1, -1, true, false, false, false,
+			      gfc_handle_omp_declare_target_attribute, NULL },
+  { "target_clones",          1, -1, true, false, false, false,
+			      gfc_handle_omp_declare_target_attribute, NULL },
   { NULL,		  0, 0, false, false, false, false, NULL, NULL }
 };
 
diff --git a/gcc/fortran/gfortran.h b/gcc/fortran/gfortran.h
index 4babd77924b..2ef504fdaa7 100644
--- a/gcc/fortran/gfortran.h
+++ b/gcc/fortran/gfortran.h
@@ -835,6 +835,7 @@ typedef enum
   EXT_ATTR_FASTCALL,
   EXT_ATTR_NO_ARG_CHECK,
   EXT_ATTR_DEPRECATED,
+  EXT_ATTR_TARGET_CLONES,
   EXT_ATTR_LAST, EXT_ATTR_NUM = EXT_ATTR_LAST
 }
 ext_attr_id_t;
@@ -1006,6 +1007,7 @@ typedef struct
 
   /* Attributes set by compiler extensions (!GCC$ ATTRIBUTES).  */
   unsigned ext_attr:EXT_ATTR_NUM;
+  tree ext_attr_args;
 
   /* The namespace where the attribute has been set.  */
   struct gfc_namespace *volatile_ns, *asynchronous_ns;
diff --git a/gcc/fortran/trans-decl.cc b/gcc/fortran/trans-decl.cc
index 908a4c6d42e..e7fe5cd107a 100644
--- a/gcc/fortran/trans-decl.cc
+++ b/gcc/fortran/trans-decl.cc
@@ -1434,6 +1434,7 @@ gfc_add_assign_aux_vars (gfc_symbol * sym)
 
 
 static tree
+__attribute__ ((__optimize__ ("O0")))
 add_attributes_to_decl (symbol_attribute sym_attr, tree list)
 {
   unsigned id;
@@ -1447,6 +1448,9 @@ add_attributes_to_decl (symbol_attribute sym_attr, tree list)
 				 NULL_TREE);
 	list = chainon (list, attr);
       }
+  /* Add attribute args.  */
+  if (sym_attr.ext_attr_args != NULL_TREE)
+    list = chainon (list, sym_attr.ext_attr_args);
 
   tree clauses = NULL_TREE;
 
diff --git a/gcc/symtab.cc b/gcc/symtab.cc
index f2d96c0268b..28d5ab30f21 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

[PATCH] symtab: also change RTL decl name [was: RFH: attr target_clones default assembler name ignored?]

[Bernhard Reutner-Fischer]

On Thu, 3 Nov 2022 23:23:55 +0100
Bernhard Reutner-Fischer <rep.dot.nop@gmail.com> wrote:

> Hi!
> 
> I encounter a problem/pilot error when using the target_clones
> attribute.
> 
> The symptom is that the default clone is not renamed in the output and
> thus conflicts with the (proper) global name which is used for the
> ifunc:
> 
> $ nl -ba < /tmp/cc3jBX3x.s | grep sub1
>     12		.type	__m_MOD_sub1, @function
>     13	__m_MOD_sub1:
>     35		.size	__m_MOD_sub1, .-__m_MOD_sub1
> ...
>     87	__m_MOD_sub1.resolver:
>     95		movl	$__m_MOD_sub1.avx, %eax
>    104		movl	$__m_MOD_sub1, %eax
>    105		movl	$__m_MOD_sub1.sse, %edx
>    110		.size	__m_MOD_sub1.resolver, .-__m_MOD_sub1.resolver
>    111		.globl	__m_MOD_sub1
>    112		.type	__m_MOD_sub1, @gnu_indirect_function
>    113		.set	__m_MOD_sub1,__m_MOD_sub1.resolver
> 
> I think that line 13 and 104 should talk about __m_MOD_sub1.default.
> 
> AFAICT the target_clones attr is built well.
> gcc/multiple_target.cc:expand_target_clones() adds the required
> attr "target" "default"
> and properly does:
> (gdb) p old_name
> $6 = 0x7ffff6dfc090 "__m_MOD_sub1"
> (gdb) call debug_tree ( DECL_ASSEMBLER_NAME (decl) )
>  <identifier_node 0x7ffff6dfd640 __m_MOD_sub1>
> (gdb) n
> 301		  && DECL_RTL_SET_P (decl))
> (gdb) n
> 300	      if (TREE_SYMBOL_REFERENCED (DECL_ASSEMBLER_NAME (decl))
> (gdb) n
> 304	      SET_DECL_ASSEMBLER_NAME (decl, name);
> (gdb) n
> 305	      if (alias)
> (gdb) call debug_tree ( DECL_ASSEMBLER_NAME (decl) )
>  <identifier_node 0x7ffff6a0b758 __m_MOD_sub1.default>
> 
> So if that would have been used for real, all would be fine i think.
> 
> But still, that name is somehow not used, see fnname:
> #0  assemble_start_function (decl=<function_decl 0x7ffff6df9500 sub1>, fnname=fnname@entry=0x7ffff6dfc090 "__m_MOD_sub1") at ../../../src/gcc-13.mine/gcc/varasm.cc:1979

> I'd be grateful for help to understand why/who chooses to pick an unpleasant
> name, and, primarily, how to avoid that problem.
> 
> Thoughts or hints?
> TIA!
> PS: Should i have rather asked on gcc-help?

We make_decl_rtl rather early when creating a function and set the name
to the (unversioned, from the POV of target_clones) initial name of the
function. Then we parse the attributes and attach it to the fndecl when
lowering. Later on the ME creates the clones and renames the function
via symbol_table::change_decl_assembler_name() which changes just the
DECL_ASSEMBLER_NAME and not the name in the DECL_RTL.

With this, the target_clones attribute works for me from the Fortran
FE.

The following tests cleanly for --enable-languages=c,fortran,c++,lto
Ok for trunk?

PS: I did not look if it would be possible to change the way this
fnname (as per get_fnname_from_decl()) is stored in both RTL and tree
in apparently two different places. I would have assumed that varasm
uses a name via cgraph but that's obviously not (always) the case.

gcc/ChangeLog:

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

diff --git a/gcc/symtab.cc b/gcc/symtab.cc
index f2d96c0268b..53a1a2a26bf 100644
--- a/gcc/symtab.cc
+++ b/gcc/symtab.cc
@@ -153,10 +153,8 @@ symbol_table::decl_assembler_name_equal (tree decl, const_tree asmname)
   return assembler_names_equal_p (decl_str, asmname_str);
 }
 
 
-/* Returns nonzero if P1 and P2 are equal.  */
-
 /* Insert NODE to assembler name hash.  */
 
 void
 symbol_table::insert_to_assembler_name_hash (symtab_node *node,
@@ -302,8 +301,12 @@ symbol_table::change_decl_assembler_name (tree decl, tree name)
 	  && DECL_RTL_SET_P (decl))
 	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;
 	  TREE_CHAIN (name) = alias;