RFH: attr target_clones default assembler name ignored?

Message ID 20221103232355.5eb1d235@nbbrfq
State New
Headers
Series RFH: attr target_clones default assembler name ignored? |

Commit Message

Bernhard Reutner-Fischer Nov. 3, 2022, 10:23 p.m. UTC
  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?
! { 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" } }
  

Patch

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