* RFH: attr target_clones default assembler name ignored?
@ 2022-11-03 22:23 Bernhard Reutner-Fischer
2022-11-04 16:39 ` [PATCH] symtab: also change RTL decl name [was: RFH: attr target_clones default assembler name ignored?] Bernhard Reutner-Fischer
0 siblings, 1 reply; 2+ messages in thread
From: Bernhard Reutner-Fischer @ 2022-11-03 22:23 UTC (permalink / raw)
To: gcc-patches; +Cc: 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
^ permalink raw reply [flat|nested] 2+ messages in thread
* [PATCH] symtab: also change RTL decl name [was: RFH: attr target_clones default assembler name ignored?]
2022-11-03 22:23 RFH: attr target_clones default assembler name ignored? Bernhard Reutner-Fischer
@ 2022-11-04 16:39 ` Bernhard Reutner-Fischer
0 siblings, 0 replies; 2+ messages in thread
From: Bernhard Reutner-Fischer @ 2022-11-04 16:39 UTC (permalink / raw)
To: gcc-patches, Jan Hubicka; +Cc: rep.dot.nop
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;
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2022-11-04 16:39 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-11-03 22:23 RFH: attr target_clones default assembler name ignored? Bernhard Reutner-Fischer
2022-11-04 16:39 ` [PATCH] symtab: also change RTL decl name [was: RFH: attr target_clones default assembler name ignored?] Bernhard Reutner-Fischer
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).