Alfie Richards <alfie.richa...@arm.com> writes:
> This changes behavior of target_clones and target_version attributes
> to be inline with what is specified in the Arm C Language Extension.
>
> Notably this changes the scope and signature of multiversioned functions
> to that of the default version, and changes the resolver to be
> created at the implementation of the default version.
>
> This is achieved by changing the C++ front end to no longer resolve any
> non-default version decls in lookup, and by moving dipatching
> for default_target sets to reuse the dispatching logic for target_clones
> in multiple_target.cc.
>
> This also fixes https://gcc.gnu.org/bugzilla/show_bug.cgi?id=118313
> for aarch64 and riscv.
>
> This changes the behavior of both the aarch64, and riscv targets.
>
> gcc/ChangeLog:
>
> * cgraphunit.cc (analyze_functions): Add dependency from default node
> to non-default versions.
> * ipa.cc (symbol_table::remove_unreachable_nodes): Ditto.
> * multiple_target.cc (ipa_target_clone): Change logic to conditionally
> dispatch target_clones and to dispatch some target_version sets.
>
> gcc/cp/ChangeLog:
>
> * call.cc (add_candidates): For target_version semantics don't resolve
> non-default versions.
> * class.cc (resolve_address_of_overloaded_function): Ditto.
> * cp-gimplify.cc (cp_genericize_r): For target_version semantics don't
> redirect calls to versioned functions (done later at
> multiple_target.cc.)
> * decl.cc (start_decl): Mangle and mark all non-default function
> decls.
> (start_preparsed_function): Ditto.
> * typeck.cc (cp_build_function_call_vec): Add error if target has no
> default implementation.
>
> gcc/testsuite/ChangeLog:
>
> * g++.target/aarch64/mv-1.C: Change for new semantics.
> * g++.target/aarch64/mv-symbols2.C: Ditto.
> * g++.target/aarch64/mv-symbols3.C: Ditto.
> * g++.target/aarch64/mv-symbols4.C: Ditto.
> * g++.target/aarch64/mv-symbols5.C: Ditto.
> * g++.target/aarch64/mvc-symbols3.C: Ditto.
> * g++.target/riscv/mv-symbols2.C: Ditto.
> * g++.target/riscv/mv-symbols3.C: Ditto.
> * g++.target/riscv/mv-symbols4.C: Ditto.
> * g++.target/riscv/mv-symbols5.C: Ditto.
> * g++.target/riscv/mvc-symbols3.C: Ditto.
> * g++.target/aarch64/mv-symbols10.C: New test.
> * g++.target/aarch64/mv-symbols11.C: New test.
> * g++.target/aarch64/mv-symbols12.C: New test.
> * g++.target/aarch64/mv-symbols14.C: New test.
> * g++.target/aarch64/mv-symbols15.C: New test.
> * g++.target/aarch64/mv-symbols6.C: New test.
> * g++.target/aarch64/mv-symbols8.C: New test.
> * g++.target/aarch64/mv-symbols9.C: New test.
Nice! I agree that the tests test for what the ACLE asks for.
Some comments below.
> ---
> gcc/cgraphunit.cc | 9 ++++
> gcc/cp/call.cc | 8 ++++
> gcc/cp/class.cc | 11 ++++-
> gcc/cp/cp-gimplify.cc | 6 ++-
> gcc/cp/decl.cc | 24 ++++++++++
> gcc/cp/typeck.cc | 8 ++++
> gcc/ipa.cc | 11 +++++
> gcc/multiple_target.cc | 13 ++++-
> gcc/testsuite/g++.target/aarch64/mv-1.C | 4 ++
> .../g++.target/aarch64/mv-symbols10.C | 43 +++++++++++++++++
> .../g++.target/aarch64/mv-symbols11.C | 27 +++++++++++
> .../g++.target/aarch64/mv-symbols12.C | 18 +++++++
> .../g++.target/aarch64/mv-symbols14.C | 16 +++++++
> .../g++.target/aarch64/mv-symbols15.C | 16 +++++++
> .../g++.target/aarch64/mv-symbols2.C | 12 ++---
> .../g++.target/aarch64/mv-symbols3.C | 6 +--
> .../g++.target/aarch64/mv-symbols4.C | 6 +--
> .../g++.target/aarch64/mv-symbols5.C | 6 +--
> .../g++.target/aarch64/mv-symbols6.C | 23 +++++++++
> .../g++.target/aarch64/mv-symbols8.C | 48 +++++++++++++++++++
> .../g++.target/aarch64/mv-symbols9.C | 46 ++++++++++++++++++
> .../g++.target/aarch64/mvc-symbols3.C | 12 ++---
> gcc/testsuite/g++.target/riscv/mv-symbols2.C | 12 ++---
> gcc/testsuite/g++.target/riscv/mv-symbols3.C | 6 +--
> gcc/testsuite/g++.target/riscv/mv-symbols4.C | 6 +--
> gcc/testsuite/g++.target/riscv/mv-symbols5.C | 6 +--
> gcc/testsuite/g++.target/riscv/mvc-symbols3.C | 12 ++---
> 27 files changed, 368 insertions(+), 47 deletions(-)
> create mode 100644 gcc/testsuite/g++.target/aarch64/mv-symbols10.C
> create mode 100644 gcc/testsuite/g++.target/aarch64/mv-symbols11.C
> create mode 100644 gcc/testsuite/g++.target/aarch64/mv-symbols12.C
> create mode 100644 gcc/testsuite/g++.target/aarch64/mv-symbols14.C
> create mode 100644 gcc/testsuite/g++.target/aarch64/mv-symbols15.C
> create mode 100644 gcc/testsuite/g++.target/aarch64/mv-symbols6.C
> create mode 100644 gcc/testsuite/g++.target/aarch64/mv-symbols8.C
> create mode 100644 gcc/testsuite/g++.target/aarch64/mv-symbols9.C
>
> diff --git a/gcc/cgraphunit.cc b/gcc/cgraphunit.cc
> index 82f205488e9..f7f8957e618 100644
> --- a/gcc/cgraphunit.cc
> +++ b/gcc/cgraphunit.cc
> @@ -1264,6 +1264,15 @@ analyze_functions (bool first_time)
> if (!cnode->analyzed)
> cnode->analyze ();
>
> + /* A reference to a default node in a funciton set implies a
function
> + refrence to all versions in the set. */
reference
> + if (cnode->function_version ()
> + && is_function_default_version (node->decl))
> + for (cgraph_function_version_info *fvi
> + = cnode->function_version ()->next;
> + fvi; fvi = fvi->next)
> + enqueue_node (fvi->this_node);
> +
Here too it seems worth caching cnode->function_version ().
> for (edge = cnode->callees; edge; edge = edge->next_callee)
> if (edge->callee->definition
> && (!DECL_EXTERNAL (edge->callee->decl)
> [...]
> diff --git a/gcc/cp/decl.cc b/gcc/cp/decl.cc
> index 9e1a2e0a9bd..552b1845352 100644
> --- a/gcc/cp/decl.cc
> +++ b/gcc/cp/decl.cc
> @@ -6174,6 +6174,12 @@ start_decl (const cp_declarator *declarator,
>
> was_public = TREE_PUBLIC (decl);
>
> + /* Mark any non-default function as versioned as it needs to be mangled
> + even when on its own in a TU. */
> + if (!TARGET_HAS_FMV_TARGET_ATTRIBUTE && TREE_CODE (decl) == FUNCTION_DECL
> + && !is_function_default_version (decl))
> + maybe_mark_function_versioned (decl);
> +
Sorry for the formatting nit, but: usually the convention is to have one
subcondition per line if the whole condition spans multiple lines.
> /* Set the assembler string for any versioned function. */
> if (TREE_CODE (decl) == FUNCTION_DECL
> && (lookup_attribute (TARGET_HAS_FMV_TARGET_ATTRIBUTE ? "target"
> @@ -6186,6 +6192,12 @@ start_decl (const cp_declarator *declarator,
> node->insert_new_function_version ();
> if (!node->function_version ()->assembler_name)
> node->function_version ()->assembler_name = DECL_ASSEMBLER_NAME (decl);
> +
> + /* In target_version semantics mangle non-default versions even if no
> + other versions are present. */
> + if (!TARGET_HAS_FMV_TARGET_ATTRIBUTE
> + && !is_function_default_version (decl))
> + maybe_mark_function_versioned (decl);
> }
>
> if ((DECL_EXTERNAL (decl) || TREE_CODE (decl) == FUNCTION_DECL)
This is probably just because I'm being lazy and not applying the
patches locally to try them out, but: could you explain why you need
to do this twice in the same function? Same for start_preparsed_function.
> [...]
> diff --git a/gcc/ipa.cc b/gcc/ipa.cc
> index 8a7b067a526..39ab0cc3de5 100644
> --- a/gcc/ipa.cc
> +++ b/gcc/ipa.cc
> @@ -433,6 +433,17 @@ symbol_table::remove_unreachable_nodes (FILE *file)
> e, &first, &reachable);
> }
> }
> +
> + /* A refrence to the default node implies use of all the other
reference
> + versions (they get used in the function resolver made later
> + in multiple_target.cc) */
> + if (cnode->function_version ()
> + && is_function_default_version (node->decl))
> + for (cgraph_function_version_info *fvi
> + = cnode->function_version ()->next;
> + fvi; fvi = fvi->next)
> + enqueue_node (fvi->this_node, &first, &reachable);
> +
I was going to complain about the cut-&-paste with cgraphunit.cc,
but I see this is actually a different enqueue_node...
> for (e = cnode->callees; e; e = e->next_callee)
> {
> symtab_node *body = e->callee->function_symbol ();
> [...]
> diff --git a/gcc/testsuite/g++.target/aarch64/mv-symbols10.C
> b/gcc/testsuite/g++.target/aarch64/mv-symbols10.C
> new file mode 100644
> index 00000000000..dc5a8ff6572
> --- /dev/null
> +++ b/gcc/testsuite/g++.target/aarch64/mv-symbols10.C
> @@ -0,0 +1,43 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O0" } */
> +/* { dg-additional-options "-Wno-experimental-fmv-target" } */
> +
> +int
> +foo ();
> +
> +int
> +foo ()
> +{
> + return 1;
> +}
> +
> +__attribute__ ((target_version ("dotprod"))) int
> +foo ()
> +{
> + return 3;
> +}
> +__attribute__ ((target_version ("sve+sve2"))) int
> +foo ()
> +{
> + return 5;
> +}
> +
> +int
> +bar ()
> +{
> + return foo ();
> +}
> +
> +/* { dg-final { scan-assembler-times "\n_Z3foov\.default:\n" 1 } } */
> +/* { dg-final { scan-assembler-times "\n_Z3foov\._Mdotprod:\n" 1 } } */
> +/* { dg-final { scan-assembler-times "\n_Z3foov\._MsveMsve2:\n" 1 } } */
> +
> +/* { dg-final { scan-assembler-times "\n_Z3foov\.resolver:\n" 1 } } */
> +/* { dg-final { scan-assembler-times "\n\tadrp\tx., _Z3foov\._MsveMsve2\n" 1
> } } */
It'd be safer to allow x[0-9]+, since the instruction could use x10 or above.
Same for other tests.
> +/* { dg-final { scan-assembler-times "\n\tadrp\tx., _Z3foov\._Mdotprod\n" 1
> } } */
> +/* { dg-final { scan-assembler-times "\n\tadrp\tx., _Z3foov\.default\n" 1 }
> } */
> +
> +/* { dg-final { scan-assembler-times "\n\tbl\t_Z3foov\n" 1 } } */
> +
> +/* { dg-final { scan-assembler-times "\n\t\.type\t_Z3foov,
> %gnu_indirect_function\n" 1 } } */
> +/* { dg-final { scan-assembler-times
> "\n\t\.set\t_Z3foov,_Z3foov\.resolver\n" 1 } } */
> [...]
> diff --git a/gcc/testsuite/g++.target/aarch64/mv-symbols12.C
> b/gcc/testsuite/g++.target/aarch64/mv-symbols12.C
> new file mode 100644
> index 00000000000..05d915f19ab
> --- /dev/null
> +++ b/gcc/testsuite/g++.target/aarch64/mv-symbols12.C
> @@ -0,0 +1,18 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O0" } */
> +/* { dg-additional-options "-Wno-experimental-fmv-target" } */
> +
> +__attribute__ ((target_version ("default"))) int
> +foo () { return 1; }
> +
> +__attribute__ ((target_version ("dotprod"))) int
> +foo () { return 3; }
> +
> +int (*test)();
> +
> +int bar ()
> +{
> + test = foo;
> +
> + return test();
> +}
What's this testing? The code seems to compile ok for unpatched compilers
and the test doesn't seem to check anything beyond that. Same question
for mv-symbols14 and mv-symbols15.
(Very minor, but: it'd be nice to renumber the tests to avoid the gap at 13,
unless it's a superstition thing.)
I don't know how much this will add in practice, but how about having a
test that contains a default version definition, a non-default version
definition, and a non-default version declaration (for a different feature)?
It looks like all the current tests have all non-default versions defined
locally or no non-default versions defined locally.
Thanks,
Richard
> diff --git a/gcc/testsuite/g++.target/aarch64/mv-symbols14.C
> b/gcc/testsuite/g++.target/aarch64/mv-symbols14.C
> new file mode 100644
> index 00000000000..fbee7800ba2
> --- /dev/null
> +++ b/gcc/testsuite/g++.target/aarch64/mv-symbols14.C
> @@ -0,0 +1,16 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O0" } */
> +/* { dg-additional-options "-Wno-experimental-fmv-target" } */
> +
> +int foo () {
> + return 1;
> +}
> +
> +void
> +bar ()
> +{
> + foo ();
> +}
> +
> +__attribute__ ((target_version ("dotprod"))) int
> +foo ();
> diff --git a/gcc/testsuite/g++.target/aarch64/mv-symbols15.C
> b/gcc/testsuite/g++.target/aarch64/mv-symbols15.C
> new file mode 100644
> index 00000000000..a10c0bfb990
> --- /dev/null
> +++ b/gcc/testsuite/g++.target/aarch64/mv-symbols15.C
> @@ -0,0 +1,16 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O0" } */
> +/* { dg-additional-options "-Wno-experimental-fmv-target" } */
> +
> +int foo () {
> + return 1;
> +}
> +
> +void bar ()
> +{
> + int (*test)() = foo;
> +
> + test();
> +}
> +
> +__attribute__ ((target_version ("dotprod"))) int foo ();
