[PATCH] Make target_clones resolver fn static.

[PATCH] Make target_clones resolver fn static.

[Martin Liška]

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

Hi.

The patch removes need to have a gnu_indirect_function global
symbol. That aligns the code with what ppc64 target does.

Patch can bootstrap on x86_64-linux-gnu and survives regression tests.

Ready to be installed?
Thanks,
Martin

gcc/ChangeLog:

2020-01-17  Martin Liska  <mliska@suse.cz>

	PR target/93274
	* config/i386/i386-features.c (make_resolver_func):
	Align the code with ppc64 target implementaion.
	We do not need to have gnu_indirect_function
	as a global function.

gcc/testsuite/ChangeLog:

2020-01-17  Martin Liska  <mliska@suse.cz>

	PR target/93274
	* gcc.target/i386/pr81213.c: Adjust to not expect
	a global unique name.
---
  gcc/config/i386/i386-features.c         | 20 +++++---------------
  gcc/testsuite/gcc.target/i386/pr81213.c |  4 ++--
  2 files changed, 7 insertions(+), 17 deletions(-)



[-- Attachment #2: 0001-Make-target_clones-resolver-fn-static.patch --]
[-- Type: text/x-patch, Size: 2254 bytes --]

diff --git a/gcc/config/i386/i386-features.c b/gcc/config/i386/i386-features.c
index e580b26b995..2517123b581 100644
--- a/gcc/config/i386/i386-features.c
+++ b/gcc/config/i386/i386-features.c
@@ -2738,26 +2738,17 @@ make_resolver_func (const tree default_decl,
 		    const tree ifunc_alias_decl,
 		    basic_block *empty_bb)
 {
-  char *resolver_name;
-  tree decl, type, decl_name, t;
+  tree decl, type, t;
 
-  /* IFUNC's have to be globally visible.  So, if the default_decl is
-     not, then the name of the IFUNC should be made unique.  */
-  if (TREE_PUBLIC (default_decl) == 0)
-    {
-      char *ifunc_name = make_unique_name (default_decl, "ifunc", true);
-      symtab->change_decl_assembler_name (ifunc_alias_decl,
-					  get_identifier (ifunc_name));
-      XDELETEVEC (ifunc_name);
-    }
-
-  resolver_name = make_unique_name (default_decl, "resolver", false);
+  /* Make the resolver function static.  The resolver function returns
+     void *.  */
+  tree decl_name = clone_function_name (default_decl, "resolver");
+  const char *resolver_name = IDENTIFIER_POINTER (decl_name);
 
   /* The resolver function should return a (void *). */
   type = build_function_type_list (ptr_type_node, NULL_TREE);
 
   decl = build_fn_decl (resolver_name, type);
-  decl_name = get_identifier (resolver_name);
   SET_DECL_ASSEMBLER_NAME (decl, decl_name);
 
   DECL_NAME (decl) = decl_name;
@@ -2809,7 +2800,6 @@ make_resolver_func (const tree default_decl,
 
   /* Create the alias for dispatch to resolver here.  */
   cgraph_node::create_same_body_alias (ifunc_alias_decl, decl);
-  XDELETEVEC (resolver_name);
   return decl;
 }
 
diff --git a/gcc/testsuite/gcc.target/i386/pr81213.c b/gcc/testsuite/gcc.target/i386/pr81213.c
index 13e15d5fef0..89c47529861 100644
--- a/gcc/testsuite/gcc.target/i386/pr81213.c
+++ b/gcc/testsuite/gcc.target/i386/pr81213.c
@@ -14,6 +14,6 @@ int main()
   return foo();
 }
 
-/* { dg-final { scan-assembler "\t.globl\tfoo\\..*\\.ifunc" } } */
+/* { dg-final { scan-assembler "\t.globl\tfoo" } } */
 /* { dg-final { scan-assembler "foo.resolver:" } } */
-/* { dg-final { scan-assembler "foo\\..*\\.ifunc, @gnu_indirect_function" } } */
+/* { dg-final { scan-assembler "foo\\, @gnu_indirect_function" } } */

Re: [PATCH] Make target_clones resolver fn static.

[Richard Biener]

On Fri, Jan 17, 2020 at 10:25 AM Martin Liška <mliska@suse.cz> wrote:
>
> Hi.
>
> The patch removes need to have a gnu_indirect_function global
> symbol. That aligns the code with what ppc64 target does.
>
> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
>
> Ready to be installed?

Did you verify the result actually works?  I'm not sure we have any runtime test
coverage for the feature and non-public functions and you don't add a testcase
either.  Maybe there's interesting coverage in the binutils or glibc testsuite
(though both might not use the compilers ifunc feature...).

The patch also suspiciously lacks removal of actually making the resolver
TREE_PUBLIC if the default implementation was not ... so I wonder whether
you verified that the resolver _is_ indeed local.

HJ, do you know anything about this requirement?  It's that way since
the original contribution of multi-versioning by Google...

Richard.

> Thanks,
> Martin
>
> gcc/ChangeLog:
>
> 2020-01-17  Martin Liska  <mliska@suse.cz>
>
>         PR target/93274
>         * config/i386/i386-features.c (make_resolver_func):
>         Align the code with ppc64 target implementaion.
>         We do not need to have gnu_indirect_function
>         as a global function.
>
> gcc/testsuite/ChangeLog:
>
> 2020-01-17  Martin Liska  <mliska@suse.cz>
>
>         PR target/93274
>         * gcc.target/i386/pr81213.c: Adjust to not expect
>         a global unique name.
> ---
>   gcc/config/i386/i386-features.c         | 20 +++++---------------
>   gcc/testsuite/gcc.target/i386/pr81213.c |  4 ++--
>   2 files changed, 7 insertions(+), 17 deletions(-)
>
>

Re: [PATCH] Make target_clones resolver fn static.

[H.J. Lu]

On Mon, Jan 20, 2020 at 2:25 AM Richard Biener
<richard.guenther@gmail.com> wrote:
>
> On Fri, Jan 17, 2020 at 10:25 AM Martin Liška <mliska@suse.cz> wrote:
> >
> > Hi.
> >
> > The patch removes need to have a gnu_indirect_function global
> > symbol. That aligns the code with what ppc64 target does.
> >
> > Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
> >
> > Ready to be installed?
>
> Did you verify the result actually works?  I'm not sure we have any runtime test
> coverage for the feature and non-public functions and you don't add a testcase
> either.  Maybe there's interesting coverage in the binutils or glibc testsuite
> (though both might not use the compilers ifunc feature...).
>
> The patch also suspiciously lacks removal of actually making the resolver
> TREE_PUBLIC if the default implementation was not ... so I wonder whether
> you verified that the resolver _is_ indeed local.
>
> HJ, do you know anything about this requirement?  It's that way since
> the original contribution of multi-versioning by Google...

We can that only if function is static:

[hjl@gnu-cfl-2 tmp]$ cat x.c
__attribute__((target_clones("avx","default")))
int
foo ()
{
  return -2;
}
[hjl@gnu-cfl-2 tmp]$ gcc -S -O2 x.c
[hjl@gnu-cfl-2 tmp]$ cat x.s
.file "x.c"
.text
.p2align 4
.type foo.default.1, @function
foo.default.1:
.LFB0:
.cfi_startproc
movl $-2, %eax
ret
.cfi_endproc
.LFE0:
.size foo.default.1, .-foo.default.1
.p2align 4
.type foo.avx.0, @function
foo.avx.0:
.LFB1:
.cfi_startproc
movl $-2, %eax
ret
.cfi_endproc
.LFE1:
.size foo.avx.0, .-foo.avx.0
.section .text.foo.resolver,"axG",@progbits,foo.resolver,comdat
.p2align 4
.weak foo.resolver
.type foo.resolver, @function
foo.resolver:
.LFB3:
.cfi_startproc
subq $8, %rsp
.cfi_def_cfa_offset 16
call __cpu_indicator_init
movl $foo.default.1, %eax
movl $foo.avx.0, %edx
testb $2, __cpu_model+13(%rip)
cmovne %rdx, %rax
addq $8, %rsp
.cfi_def_cfa_offset 8
ret
.cfi_endproc
.LFE3:
.size foo.resolver, .-foo.resolver
.globl foo
.type foo, @gnu_indirect_function
.set foo,foo.resolver
.ident "GCC: (GNU) 9.2.1 20191120 (Red Hat 9.2.1-2)"
.section .note.GNU-stack,"",@progbits
[hjl@gnu-cfl-2 tmp]$

In this case, foo must be global.

> Richard.
>
> > Thanks,
> > Martin
> >
> > gcc/ChangeLog:
> >
> > 2020-01-17  Martin Liska  <mliska@suse.cz>
> >
> >         PR target/93274
> >         * config/i386/i386-features.c (make_resolver_func):
> >         Align the code with ppc64 target implementaion.
> >         We do not need to have gnu_indirect_function
> >         as a global function.
> >
> > gcc/testsuite/ChangeLog:
> >
> > 2020-01-17  Martin Liska  <mliska@suse.cz>
> >
> >         PR target/93274
> >         * gcc.target/i386/pr81213.c: Adjust to not expect
> >         a global unique name.
> > ---
> >   gcc/config/i386/i386-features.c         | 20 +++++---------------
> >   gcc/testsuite/gcc.target/i386/pr81213.c |  4 ++--
> >   2 files changed, 7 insertions(+), 17 deletions(-)
> >
> >



-- 
H.J.

Re: [PATCH] Make target_clones resolver fn static.

[Alexander Monakov]

On Mon, 20 Jan 2020, H.J. Lu wrote:
> We can that only if function is static:
> 
[ship asm]
> 
> In this case, foo must be global.

H.J., can you rephrase more clearly? Your response seems contradictory and
does not help to explain the matter.

Alexander

Re: [PATCH] Make target_clones resolver fn static.

[H.J. Lu]

On Mon, Jan 20, 2020 at 5:36 AM Alexander Monakov <amonakov@ispras.ru> wrote:
>
>
>
> On Mon, 20 Jan 2020, H.J. Lu wrote:
> > We can that only if function is static:
> >
> [ship asm]
> >
> > In this case, foo must be global.
>
> H.J., can you rephrase more clearly? Your response seems contradictory and
> does not help to explain the matter.
>
> Alexander

For,

---
__attribute__((target_clones("avx","default")))
int
foo ()
{
  return -2;
}
----

foo's resolver must be global.  For

---
__attribute__((target_clones("avx","default")))
static int
foo ()
{
  return -2;
}
---

foo's resolver must be static.

-- 
H.J.

Re: [PATCH] Make target_clones resolver fn static.

[Alexander Monakov]

On Mon, 20 Jan 2020, H.J. Lu wrote:
> For,
> 
> ---
> __attribute__((target_clones("avx","default")))
> int
> foo ()
> {
>   return -2;
> }
> ----
> 
> foo's resolver must be global.  For
> 
> ---
> __attribute__((target_clones("avx","default")))
> static int
> foo ()
> {
>   return -2;
> }
> ---
> 
> foo's resolver must be static.

Bare IFUNC's don't seem to have this restriction. Why do we want to
constrain target clones this way?

Alexander

Re: [PATCH] Make target_clones resolver fn static.

[H.J. Lu]

On Mon, Jan 20, 2020 at 6:16 AM Alexander Monakov <amonakov@ispras.ru> wrote:
>
>
>
> On Mon, 20 Jan 2020, H.J. Lu wrote:
> > For,
> >
> > ---
> > __attribute__((target_clones("avx","default")))
> > int
> > foo ()
> > {
> >   return -2;
> > }
> > ----
> >
> > foo's resolver must be global.  For
> >
> > ---
> > __attribute__((target_clones("avx","default")))
> > static int
> > foo ()
> > {
> >   return -2;
> > }
> > ---
> >
> > foo's resolver must be static.
>
> Bare IFUNC's don't seem to have this restriction. Why do we want to
> constrain target clones this way?
>

foo's resolver acts as foo.  It should have the same visibility as foo.


-- 
H.J.

Re: [PATCH] Make target_clones resolver fn static.

[Alexander Monakov]

On Mon, 20 Jan 2020, H.J. Lu wrote:

> > Bare IFUNC's don't seem to have this restriction. Why do we want to
> > constrain target clones this way?
> >
> 
> foo's resolver acts as foo.  It should have the same visibility as foo.

What do you mean by that? From the implementation standpoint, there's
two symbols of different type with the same value. There's no problem
allowing one of them have local binding and the other have global binding.

Is there something special about target clones that doesn't come into
play with ifuncs?

Alexander

Re: [PATCH] Make target_clones resolver fn static.

[H.J. Lu]

On Mon, Jan 20, 2020 at 6:41 AM Alexander Monakov <amonakov@ispras.ru> wrote:
>
>
>
> On Mon, 20 Jan 2020, H.J. Lu wrote:
>
> > > Bare IFUNC's don't seem to have this restriction. Why do we want to
> > > constrain target clones this way?
> > >
> >
> > foo's resolver acts as foo.  It should have the same visibility as foo.
>
> What do you mean by that? From the implementation standpoint, there's
> two symbols of different type with the same value. There's no problem
> allowing one of them have local binding and the other have global binding.
>
> Is there something special about target clones that doesn't come into
> play with ifuncs?
>

I stand corrected.   Resolver should be static and it shouldn't be weak.


-- 
H.J.

Re: [PATCH] Make target_clones resolver fn static.

[Richard Biener]

On Mon, Jan 20, 2020 at 3:46 PM H.J. Lu <hjl.tools@gmail.com> wrote:
>
> On Mon, Jan 20, 2020 at 6:41 AM Alexander Monakov <amonakov@ispras.ru> wrote:
> >
> >
> >
> > On Mon, 20 Jan 2020, H.J. Lu wrote:
> >
> > > > Bare IFUNC's don't seem to have this restriction. Why do we want to
> > > > constrain target clones this way?
> > > >
> > >
> > > foo's resolver acts as foo.  It should have the same visibility as foo.
> >
> > What do you mean by that? From the implementation standpoint, there's
> > two symbols of different type with the same value. There's no problem
> > allowing one of them have local binding and the other have global binding.
> >
> > Is there something special about target clones that doesn't come into
> > play with ifuncs?
> >
>
> I stand corrected.   Resolver should be static and it shouldn't be weak.

Reading the patch again and looking up more context it seems that the resolver
is already static we just mangle it extra when the original function
is public (?)
If so the patch looks quite obvious to me if we use some character not valid
in indetifiers in C but valid in assembly for the resolver decl.

Richard.

>
> --
> H.J.

Re: [PATCH] Make target_clones resolver fn static.

[Martin Liška]

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

On 1/20/20 3:52 PM, Richard Biener wrote:
> On Mon, Jan 20, 2020 at 3:46 PM H.J. Lu <hjl.tools@gmail.com> wrote:
>>
>> On Mon, Jan 20, 2020 at 6:41 AM Alexander Monakov <amonakov@ispras.ru> wrote:
>>>
>>>
>>>
>>> On Mon, 20 Jan 2020, H.J. Lu wrote:
>>>
>>>>> Bare IFUNC's don't seem to have this restriction. Why do we want to
>>>>> constrain target clones this way?
>>>>>
>>>>
>>>> foo's resolver acts as foo.  It should have the same visibility as foo.
>>>
>>> What do you mean by that? From the implementation standpoint, there's
>>> two symbols of different type with the same value. There's no problem
>>> allowing one of them have local binding and the other have global binding.
>>>
>>> Is there something special about target clones that doesn't come into
>>> play with ifuncs?
>>>
>>
>> I stand corrected.   Resolver should be static and it shouldn't be weak.
> 
> Reading the patch again and looking up more context it seems that the resolver
> is already static we just mangle it extra when the original function
> is public (?)
> If so the patch looks quite obvious to me if we use some character not valid
> in indetifiers in C but valid in assembly for the resolver decl.

Hello.

I'm sending updated version of the patch where I'm adding a run-time test
for 2 static symbols (with the same name) and it works fine. Moreover
I'm newly setting TREE_PUBLIC (ifunc_alias_decl) = 0 which seems to
work properly.

I tested both x86_64-linux-gnu and ppc64le-linux-gnu.

Martin

> 
> Richard.
> 
>>
>> --
>> H.J.


[-- Attachment #2: 0001-Make-target_clones-resolver-fn-static.patch --]
[-- Type: text/x-patch, Size: 5362 bytes --]

From a3faaced989869867671ceadd89b56fabde225ff Mon Sep 17 00:00:00 2001
From: Martin Liska <mliska@suse.cz>
Date: Thu, 16 Jan 2020 10:38:41 +0100
Subject: [PATCH] Make target_clones resolver fn static.

gcc/ChangeLog:

2020-01-17  Martin Liska  <mliska@suse.cz>

	PR target/93274
	* config/i386/i386-features.c (make_resolver_func):
	Align the code with ppc64 target implementaion.
	We do not need to have gnu_indirect_function
	as a global function.  Drop TREE_PUBLIC on
	ifunc symbol.
	* config/rs6000/rs6000.c (make_resolver_func): Drop
	TREE_PUBLIC on ifunc symbol.

gcc/testsuite/ChangeLog:

2020-01-17  Martin Liska  <mliska@suse.cz>

	PR target/93274
	* gcc.target/i386/pr81213.c: Adjust to not expect
	a global unique name.
	* gcc.target/i386/pr81213-2.c: New test.
---
 gcc/config/i386/i386-features.c           | 23 ++++++++---------------
 gcc/config/rs6000/rs6000.c                | 12 ++++++++++++
 gcc/testsuite/gcc.target/i386/pr81213-2.c | 11 +++++++++++
 gcc/testsuite/gcc.target/i386/pr81213.c   | 11 +++++++----
 4 files changed, 38 insertions(+), 19 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/i386/pr81213-2.c

diff --git a/gcc/config/i386/i386-features.c b/gcc/config/i386/i386-features.c
index e580b26b995..07eca286544 100644
--- a/gcc/config/i386/i386-features.c
+++ b/gcc/config/i386/i386-features.c
@@ -2738,26 +2738,17 @@ make_resolver_func (const tree default_decl,
 		    const tree ifunc_alias_decl,
 		    basic_block *empty_bb)
 {
-  char *resolver_name;
-  tree decl, type, decl_name, t;
+  tree decl, type, t;
 
-  /* IFUNC's have to be globally visible.  So, if the default_decl is
-     not, then the name of the IFUNC should be made unique.  */
-  if (TREE_PUBLIC (default_decl) == 0)
-    {
-      char *ifunc_name = make_unique_name (default_decl, "ifunc", true);
-      symtab->change_decl_assembler_name (ifunc_alias_decl,
-					  get_identifier (ifunc_name));
-      XDELETEVEC (ifunc_name);
-    }
-
-  resolver_name = make_unique_name (default_decl, "resolver", false);
+  /* Make the resolver function static.  The resolver function returns
+     void *.  */
+  tree decl_name = clone_function_name (default_decl, "resolver");
+  const char *resolver_name = IDENTIFIER_POINTER (decl_name);
 
   /* The resolver function should return a (void *). */
   type = build_function_type_list (ptr_type_node, NULL_TREE);
 
   decl = build_fn_decl (resolver_name, type);
-  decl_name = get_identifier (resolver_name);
   SET_DECL_ASSEMBLER_NAME (decl, decl_name);
 
   DECL_NAME (decl) = decl_name;
@@ -2784,6 +2775,9 @@ make_resolver_func (const tree default_decl,
       DECL_COMDAT (decl) = 1;
       make_decl_one_only (decl, DECL_ASSEMBLER_NAME (decl));
     }
+  else
+    TREE_PUBLIC (ifunc_alias_decl) = 0;
+
   /* Build result decl and add to function_decl. */
   t = build_decl (UNKNOWN_LOCATION, RESULT_DECL, NULL_TREE, ptr_type_node);
   DECL_CONTEXT (t) = decl;
@@ -2809,7 +2803,6 @@ make_resolver_func (const tree default_decl,
 
   /* Create the alias for dispatch to resolver here.  */
   cgraph_node::create_same_body_alias (ifunc_alias_decl, decl);
-  XDELETEVEC (resolver_name);
   return decl;
 }
 
diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
index 127927d9583..97b8ebe4d80 100644
--- a/gcc/config/rs6000/rs6000.c
+++ b/gcc/config/rs6000/rs6000.c
@@ -23858,6 +23858,18 @@ make_resolver_func (const tree default_decl,
   DECL_INITIAL (decl) = make_node (BLOCK);
   DECL_STATIC_CONSTRUCTOR (decl) = 0;
 
+  if (DECL_COMDAT_GROUP (default_decl)
+      || TREE_PUBLIC (default_decl))
+    {
+      /* In this case, each translation unit with a call to this
+	 versioned function will put out a resolver.  Ensure it
+	 is comdat to keep just one copy.  */
+      DECL_COMDAT (decl) = 1;
+      make_decl_one_only (decl, DECL_ASSEMBLER_NAME (decl));
+    }
+  else
+    TREE_PUBLIC (dispatch_decl) = 0;
+
   /* Build result decl and add to function_decl.  */
   tree t = build_decl (UNKNOWN_LOCATION, RESULT_DECL, NULL_TREE, ptr_type_node);
   DECL_CONTEXT (t) = decl;
diff --git a/gcc/testsuite/gcc.target/i386/pr81213-2.c b/gcc/testsuite/gcc.target/i386/pr81213-2.c
new file mode 100644
index 00000000000..a80622cb184
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/pr81213-2.c
@@ -0,0 +1,11 @@
+__attribute__((target_clones("avx","arch=slm","arch=core-avx2","default")))
+static int
+foo ()
+{
+  return 2;
+}
+
+int bar()
+{
+  return foo();
+}
diff --git a/gcc/testsuite/gcc.target/i386/pr81213.c b/gcc/testsuite/gcc.target/i386/pr81213.c
index 13e15d5fef0..334838631d0 100644
--- a/gcc/testsuite/gcc.target/i386/pr81213.c
+++ b/gcc/testsuite/gcc.target/i386/pr81213.c
@@ -1,6 +1,9 @@
 /* PR ipa/81214.  */
-/* { dg-do compile } */
+/* { dg-do run } */
 /* { dg-require-ifunc "" } */
+/* { dg-additional-sources "pr81213-2.c" } */
+
+int bar();
 
 __attribute__((target_clones("avx","arch=slm","arch=core-avx2","default")))
 static int
@@ -11,9 +14,9 @@ foo ()
 
 int main()
 {
-  return foo();
+  return foo() + bar();
 }
 
-/* { dg-final { scan-assembler "\t.globl\tfoo\\..*\\.ifunc" } } */
+/* { dg-final { scan-assembler "\t.globl\tfoo" } } */
 /* { dg-final { scan-assembler "foo.resolver:" } } */
-/* { dg-final { scan-assembler "foo\\..*\\.ifunc, @gnu_indirect_function" } } */
+/* { dg-final { scan-assembler "foo\\, @gnu_indirect_function" } } */
-- 
2.24.1

Re: [PATCH] Make target_clones resolver fn static.

[Richard Biener]

On Tue, Jan 21, 2020 at 1:48 PM Martin Liška <mliska@suse.cz> wrote:
>
> On 1/20/20 3:52 PM, Richard Biener wrote:
> > On Mon, Jan 20, 2020 at 3:46 PM H.J. Lu <hjl.tools@gmail.com> wrote:
> >>
> >> On Mon, Jan 20, 2020 at 6:41 AM Alexander Monakov <amonakov@ispras.ru> wrote:
> >>>
> >>>
> >>>
> >>> On Mon, 20 Jan 2020, H.J. Lu wrote:
> >>>
> >>>>> Bare IFUNC's don't seem to have this restriction. Why do we want to
> >>>>> constrain target clones this way?
> >>>>>
> >>>>
> >>>> foo's resolver acts as foo.  It should have the same visibility as foo.
> >>>
> >>> What do you mean by that? From the implementation standpoint, there's
> >>> two symbols of different type with the same value. There's no problem
> >>> allowing one of them have local binding and the other have global binding.
> >>>
> >>> Is there something special about target clones that doesn't come into
> >>> play with ifuncs?
> >>>
> >>
> >> I stand corrected.   Resolver should be static and it shouldn't be weak.
> >
> > Reading the patch again and looking up more context it seems that the resolver
> > is already static we just mangle it extra when the original function
> > is public (?)
> > If so the patch looks quite obvious to me if we use some character not valid
> > in indetifiers in C but valid in assembly for the resolver decl.
>
> Hello.
>
> I'm sending updated version of the patch where I'm adding a run-time test
> for 2 static symbols (with the same name) and it works fine. Moreover
> I'm newly setting TREE_PUBLIC (ifunc_alias_decl) = 0 which seems to
> work properly.
>
> I tested both x86_64-linux-gnu and ppc64le-linux-gnu.

So this doesn't help review including two different target changes.  Making
ifunc dispatchers of public functions non-public looks like an unrelated thing
to the bug (sorry if I mis-suggested).  So I feel comfortable approving the
earlier patch which just dropped the extra mangling for non-public dispatchers
in the x86 backend.  The other thing looks like sth for next stage1?

Thanks,
Richard.

> Martin
>
> >
> > Richard.
> >
> >>
> >> --
> >> H.J.
>

Re: [PATCH] Make target_clones resolver fn static.

[Martin Liška]

On 1/23/20 2:09 PM, Richard Biener wrote:
> On Tue, Jan 21, 2020 at 1:48 PM Martin Liška <mliska@suse.cz> wrote:
>>
>> On 1/20/20 3:52 PM, Richard Biener wrote:
>>> On Mon, Jan 20, 2020 at 3:46 PM H.J. Lu <hjl.tools@gmail.com> wrote:
>>>>
>>>> On Mon, Jan 20, 2020 at 6:41 AM Alexander Monakov <amonakov@ispras.ru> wrote:
>>>>>
>>>>>
>>>>>
>>>>> On Mon, 20 Jan 2020, H.J. Lu wrote:
>>>>>
>>>>>>> Bare IFUNC's don't seem to have this restriction. Why do we want to
>>>>>>> constrain target clones this way?
>>>>>>>
>>>>>>
>>>>>> foo's resolver acts as foo.  It should have the same visibility as foo.
>>>>>
>>>>> What do you mean by that? From the implementation standpoint, there's
>>>>> two symbols of different type with the same value. There's no problem
>>>>> allowing one of them have local binding and the other have global binding.
>>>>>
>>>>> Is there something special about target clones that doesn't come into
>>>>> play with ifuncs?
>>>>>
>>>>
>>>> I stand corrected.   Resolver should be static and it shouldn't be weak.
>>>
>>> Reading the patch again and looking up more context it seems that the resolver
>>> is already static we just mangle it extra when the original function
>>> is public (?)
>>> If so the patch looks quite obvious to me if we use some character not valid
>>> in indetifiers in C but valid in assembly for the resolver decl.
>>
>> Hello.
>>
>> I'm sending updated version of the patch where I'm adding a run-time test
>> for 2 static symbols (with the same name) and it works fine. Moreover
>> I'm newly setting TREE_PUBLIC (ifunc_alias_decl) = 0 which seems to
>> work properly.
>>
>> I tested both x86_64-linux-gnu and ppc64le-linux-gnu.
> 
> So this doesn't help review including two different target changes.  Making
> ifunc dispatchers of public functions non-public looks like an unrelated thing
> to the bug (sorry if I mis-suggested).  So I feel comfortable approving the
> earlier patch which just dropped the extra mangling for non-public dispatchers
> in the x86 backend.

Works for me.

>  The other thing looks like sth for next stage1?

Yes.

Martin

> 
> Thanks,
> Richard.
> 
>> Martin
>>
>>>
>>> Richard.
>>>
>>>>
>>>> --
>>>> H.J.
>>

Re: [PATCH] Make target_clones resolver fn static.

[Alexander Monakov]

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



On Thu, 23 Jan 2020, Martin Liška wrote:
> > So this doesn't help review including two different target changes.  Making
> > ifunc dispatchers of public functions non-public looks like an unrelated
> > thing
> > to the bug (sorry if I mis-suggested).  So I feel comfortable approving the
> > earlier patch which just dropped the extra mangling for non-public
> > dispatchers
> > in the x86 backend.
> 
> Works for me.

If you will be revising the patch, can you please improve the new comment?

I mean this addition:

  /* Make the resolver function static.
  ... */

but it's not what the following code does.

Thanks.
Alexander

Re: [PATCH] Make target_clones resolver fn static.

[Martin Liška]

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

On 1/23/20 2:52 PM, Alexander Monakov wrote:
> 
> 
> On Thu, 23 Jan 2020, Martin Liška wrote:
>>> So this doesn't help review including two different target changes.  Making
>>> ifunc dispatchers of public functions non-public looks like an unrelated
>>> thing
>>> to the bug (sorry if I mis-suggested).  So I feel comfortable approving the
>>> earlier patch which just dropped the extra mangling for non-public
>>> dispatchers
>>> in the x86 backend.
>>
>> Works for me.
> 
> If you will be revising the patch, can you please improve the new comment?
> 
> I mean this addition:
> 
>    /* Make the resolver function static.
>    ... */
> 
> but it's not what the following code does.

Sure, there's original version of the patch with modified comment.
I'm going to install it.

Martin

> 
> Thanks.
> Alexander
> 


[-- Attachment #2: 0001-Do-not-generate-a-unique-fnname-for-resolver.patch --]
[-- Type: text/x-patch, Size: 3018 bytes --]

From 256ba0077dd91f70117c8c65f9ffd206ea98b2c4 Mon Sep 17 00:00:00 2001
From: Martin Liska <mliska@suse.cz>
Date: Thu, 16 Jan 2020 10:38:41 +0100
Subject: [PATCH] Do not generate a unique fnname for resolver.

gcc/ChangeLog:

2020-01-17  Martin Liska  <mliska@suse.cz>

	PR target/93274
	* config/i386/i386-features.c (make_resolver_func):
	Align the code with ppc64 target implementation.
	Do not generate a unique name for resolver function.

gcc/testsuite/ChangeLog:

2020-01-17  Martin Liska  <mliska@suse.cz>

	PR target/93274
	* gcc.target/i386/pr81213.c: Adjust to not expect
	a globally unique name.
---
 gcc/config/i386/i386-features.c         | 19 ++++---------------
 gcc/testsuite/gcc.target/i386/pr81213.c |  4 ++--
 2 files changed, 6 insertions(+), 17 deletions(-)

diff --git a/gcc/config/i386/i386-features.c b/gcc/config/i386/i386-features.c
index e580b26b995..b49e6f8d408 100644
--- a/gcc/config/i386/i386-features.c
+++ b/gcc/config/i386/i386-features.c
@@ -2738,26 +2738,16 @@ make_resolver_func (const tree default_decl,
 		    const tree ifunc_alias_decl,
 		    basic_block *empty_bb)
 {
-  char *resolver_name;
-  tree decl, type, decl_name, t;
+  tree decl, type, t;
 
-  /* IFUNC's have to be globally visible.  So, if the default_decl is
-     not, then the name of the IFUNC should be made unique.  */
-  if (TREE_PUBLIC (default_decl) == 0)
-    {
-      char *ifunc_name = make_unique_name (default_decl, "ifunc", true);
-      symtab->change_decl_assembler_name (ifunc_alias_decl,
-					  get_identifier (ifunc_name));
-      XDELETEVEC (ifunc_name);
-    }
-
-  resolver_name = make_unique_name (default_decl, "resolver", false);
+  /* Create resolver function name based on default_decl.  */
+  tree decl_name = clone_function_name (default_decl, "resolver");
+  const char *resolver_name = IDENTIFIER_POINTER (decl_name);
 
   /* The resolver function should return a (void *). */
   type = build_function_type_list (ptr_type_node, NULL_TREE);
 
   decl = build_fn_decl (resolver_name, type);
-  decl_name = get_identifier (resolver_name);
   SET_DECL_ASSEMBLER_NAME (decl, decl_name);
 
   DECL_NAME (decl) = decl_name;
@@ -2809,7 +2799,6 @@ make_resolver_func (const tree default_decl,
 
   /* Create the alias for dispatch to resolver here.  */
   cgraph_node::create_same_body_alias (ifunc_alias_decl, decl);
-  XDELETEVEC (resolver_name);
   return decl;
 }
 
diff --git a/gcc/testsuite/gcc.target/i386/pr81213.c b/gcc/testsuite/gcc.target/i386/pr81213.c
index 13e15d5fef0..89c47529861 100644
--- a/gcc/testsuite/gcc.target/i386/pr81213.c
+++ b/gcc/testsuite/gcc.target/i386/pr81213.c
@@ -14,6 +14,6 @@ int main()
   return foo();
 }
 
-/* { dg-final { scan-assembler "\t.globl\tfoo\\..*\\.ifunc" } } */
+/* { dg-final { scan-assembler "\t.globl\tfoo" } } */
 /* { dg-final { scan-assembler "foo.resolver:" } } */
-/* { dg-final { scan-assembler "foo\\..*\\.ifunc, @gnu_indirect_function" } } */
+/* { dg-final { scan-assembler "foo\\, @gnu_indirect_function" } } */
-- 
2.25.0

[stage1] [PATCH] Make target_clones resolver fn static if possible.

[Martin Liška]

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

On 1/23/20 2:09 PM, Richard Biener wrote:
>   The other thing looks like sth for next stage1?

Hi.

I'm sending the second part for next stage1.

Patch can bootstrap on x86_64-linux-gnu and survives regression tests.

Ready to be installed in next stage1?
Thanks,
Martin

[-- Attachment #2: 0001-Make-target_clones-resolver-fn-static-if-possible.patch --]
[-- Type: text/x-patch, Size: 3507 bytes --]

From 6d2113da263d7a6a0bb0adcf22edb074646f1996 Mon Sep 17 00:00:00 2001
From: Martin Liska <mliska@suse.cz>
Date: Thu, 16 Jan 2020 10:38:41 +0100
Subject: [PATCH] Make target_clones resolver fn static if possible.

gcc/ChangeLog:

2020-03-17  Martin Liska  <mliska@suse.cz>

	PR target/93274
	* config/i386/i386-features.c (make_resolver_func): Drop
	public flag for resolver.
	* config/rs6000/rs6000.c (make_resolver_func): Add comdat
	group for resolver and drop public flag if possible.

gcc/testsuite/ChangeLog:

2020-03-17  Martin Liska  <mliska@suse.cz>

	PR target/93274
	* gcc.target/i386/pr81213-2.c: New test.
	* gcc.target/i386/pr81213.c: Add additional source.
---
 gcc/config/i386/i386-features.c           |  3 +++
 gcc/config/rs6000/rs6000.c                | 12 ++++++++++++
 gcc/testsuite/gcc.target/i386/pr81213-2.c | 11 +++++++++++
 gcc/testsuite/gcc.target/i386/pr81213.c   |  7 +++++--
 4 files changed, 31 insertions(+), 2 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/i386/pr81213-2.c

diff --git a/gcc/config/i386/i386-features.c b/gcc/config/i386/i386-features.c
index 6919c839605..690e6373c27 100644
--- a/gcc/config/i386/i386-features.c
+++ b/gcc/config/i386/i386-features.c
@@ -2777,6 +2777,9 @@ make_resolver_func (const tree default_decl,
       DECL_COMDAT (decl) = 1;
       make_decl_one_only (decl, DECL_ASSEMBLER_NAME (decl));
     }
+  else
+    TREE_PUBLIC (ifunc_alias_decl) = 0;
+
   /* Build result decl and add to function_decl. */
   t = build_decl (UNKNOWN_LOCATION, RESULT_DECL, NULL_TREE, ptr_type_node);
   DECL_CONTEXT (t) = decl;
diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
index 5798f924472..0911ab019cd 100644
--- a/gcc/config/rs6000/rs6000.c
+++ b/gcc/config/rs6000/rs6000.c
@@ -23887,6 +23887,18 @@ make_resolver_func (const tree default_decl,
   DECL_INITIAL (decl) = make_node (BLOCK);
   DECL_STATIC_CONSTRUCTOR (decl) = 0;
 
+  if (DECL_COMDAT_GROUP (default_decl)
+      || TREE_PUBLIC (default_decl))
+    {
+      /* In this case, each translation unit with a call to this
+	 versioned function will put out a resolver.  Ensure it
+	 is comdat to keep just one copy.  */
+      DECL_COMDAT (decl) = 1;
+      make_decl_one_only (decl, DECL_ASSEMBLER_NAME (decl));
+    }
+  else
+    TREE_PUBLIC (dispatch_decl) = 0;
+
   /* Build result decl and add to function_decl.  */
   tree t = build_decl (UNKNOWN_LOCATION, RESULT_DECL, NULL_TREE, ptr_type_node);
   DECL_CONTEXT (t) = decl;
diff --git a/gcc/testsuite/gcc.target/i386/pr81213-2.c b/gcc/testsuite/gcc.target/i386/pr81213-2.c
new file mode 100644
index 00000000000..a80622cb184
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/pr81213-2.c
@@ -0,0 +1,11 @@
+__attribute__((target_clones("avx","arch=slm","arch=core-avx2","default")))
+static int
+foo ()
+{
+  return 2;
+}
+
+int bar()
+{
+  return foo();
+}
diff --git a/gcc/testsuite/gcc.target/i386/pr81213.c b/gcc/testsuite/gcc.target/i386/pr81213.c
index 89c47529861..334838631d0 100644
--- a/gcc/testsuite/gcc.target/i386/pr81213.c
+++ b/gcc/testsuite/gcc.target/i386/pr81213.c
@@ -1,6 +1,9 @@
 /* PR ipa/81214.  */
-/* { dg-do compile } */
+/* { dg-do run } */
 /* { dg-require-ifunc "" } */
+/* { dg-additional-sources "pr81213-2.c" } */
+
+int bar();
 
 __attribute__((target_clones("avx","arch=slm","arch=core-avx2","default")))
 static int
@@ -11,7 +14,7 @@ foo ()
 
 int main()
 {
-  return foo();
+  return foo() + bar();
 }
 
 /* { dg-final { scan-assembler "\t.globl\tfoo" } } */
-- 
2.25.1

Re: [stage1] [PATCH] Make target_clones resolver fn static if possible.

[Martin Liška]

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

Hi.

The patch was supposed to be a next stage1 material, but then PR94271
was reported. The patch for it includes the hunk and one more that
leaves unique_name and resolution for the "default" clone. That will
align the symbol with other target clones and it's name will become
privatized eventually in LTO.

Patch can bootstrap on x86_64-linux-gnu and survives regression tests.

Ready to be installed?
Thanks,
Martin

[-- Attachment #2: 0001-Make-target_clones-resolver-fn-static-if-possible.patch --]
[-- Type: text/x-patch, Size: 5455 bytes --]

From 1431a34f70128bdce59c94dad1d10f91673f63eb Mon Sep 17 00:00:00 2001
From: Martin Liska <mliska@suse.cz>
Date: Thu, 16 Jan 2020 10:38:41 +0100
Subject: [PATCH] Make target_clones resolver fn static if possible.

gcc/ChangeLog:

2020-03-17  Martin Liska  <mliska@suse.cz>

	PR target/93274
	PR ipa/94271
	* config/i386/i386-features.c (make_resolver_func): Drop
	public flag for resolver.
	* config/rs6000/rs6000.c (make_resolver_func): Add comdat
	group for resolver and drop public flag if possible.
	* multiple_target.c (create_dispatcher_calls): Drop unique_name
	and resolution as we want to enable LTO privatization of the default
	symbol.

gcc/testsuite/ChangeLog:

2020-03-17  Martin Liska  <mliska@suse.cz>

	PR target/93274	PR lto/94271
	* gcc.target/i386/pr81213-2.c: New test.
	* gcc.target/i386/pr81213.c: Add additional source.
	* gcc.dg/lto/pr94271_0.c: New test.
	* gcc.dg/lto/pr94271_1.c: New test.
---
 gcc/config/i386/i386-features.c           |  3 +++
 gcc/config/rs6000/rs6000.c                | 12 ++++++++++++
 gcc/multiple_target.c                     |  4 ----
 gcc/testsuite/gcc.dg/lto/pr94271_0.c      | 13 +++++++++++++
 gcc/testsuite/gcc.dg/lto/pr94271_1.c      | 17 +++++++++++++++++
 gcc/testsuite/gcc.target/i386/pr81213-2.c | 11 +++++++++++
 gcc/testsuite/gcc.target/i386/pr81213.c   |  7 +++++--
 7 files changed, 61 insertions(+), 6 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/lto/pr94271_0.c
 create mode 100644 gcc/testsuite/gcc.dg/lto/pr94271_1.c
 create mode 100644 gcc/testsuite/gcc.target/i386/pr81213-2.c

diff --git a/gcc/config/i386/i386-features.c b/gcc/config/i386/i386-features.c
index 6528832487e..3c70279dc7c 100644
--- a/gcc/config/i386/i386-features.c
+++ b/gcc/config/i386/i386-features.c
@@ -2777,6 +2777,9 @@ make_resolver_func (const tree default_decl,
       DECL_COMDAT (decl) = 1;
       make_decl_one_only (decl, DECL_ASSEMBLER_NAME (decl));
     }
+  else
+    TREE_PUBLIC (ifunc_alias_decl) = 0;
+
   /* Build result decl and add to function_decl. */
   t = build_decl (UNKNOWN_LOCATION, RESULT_DECL, NULL_TREE, ptr_type_node);
   DECL_CONTEXT (t) = decl;
diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
index 07f7cf516ba..7505a0e1e8e 100644
--- a/gcc/config/rs6000/rs6000.c
+++ b/gcc/config/rs6000/rs6000.c
@@ -23894,6 +23894,18 @@ make_resolver_func (const tree default_decl,
   DECL_INITIAL (decl) = make_node (BLOCK);
   DECL_STATIC_CONSTRUCTOR (decl) = 0;
 
+  if (DECL_COMDAT_GROUP (default_decl)
+      || TREE_PUBLIC (default_decl))
+    {
+      /* In this case, each translation unit with a call to this
+	 versioned function will put out a resolver.  Ensure it
+	 is comdat to keep just one copy.  */
+      DECL_COMDAT (decl) = 1;
+      make_decl_one_only (decl, DECL_ASSEMBLER_NAME (decl));
+    }
+  else
+    TREE_PUBLIC (dispatch_decl) = 0;
+
   /* Build result decl and add to function_decl.  */
   tree t = build_decl (UNKNOWN_LOCATION, RESULT_DECL, NULL_TREE, ptr_type_node);
   DECL_CONTEXT (t) = decl;
diff --git a/gcc/multiple_target.c b/gcc/multiple_target.c
index cccfd2774db..c1cfe8ff978 100644
--- a/gcc/multiple_target.c
+++ b/gcc/multiple_target.c
@@ -178,10 +178,6 @@ create_dispatcher_calls (struct cgraph_node *node)
   node->externally_visible = false;
   node->forced_by_abi = false;
   node->set_section (NULL);
-  node->unique_name = ((node->resolution == LDPR_PREVAILING_DEF_IRONLY
-			|| node->resolution == LDPR_PREVAILING_DEF_IRONLY_EXP)
-		       && !flag_incremental_link);
-  node->resolution = LDPR_PREVAILING_DEF_IRONLY;
 
   DECL_ARTIFICIAL (node->decl) = 1;
   node->force_output = true;
diff --git a/gcc/testsuite/gcc.dg/lto/pr94271_0.c b/gcc/testsuite/gcc.dg/lto/pr94271_0.c
new file mode 100644
index 00000000000..2ce7d65411a
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/lto/pr94271_0.c
@@ -0,0 +1,13 @@
+/* PR lto/94271 */
+/* { dg-lto-do link } */
+
+int a;
+
+static int __attribute__ ((target_clones ("default", "avx512f"))) fast_clamp ()
+{}
+
+void
+c ()
+{
+  a = fast_clamp ();
+}
diff --git a/gcc/testsuite/gcc.dg/lto/pr94271_1.c b/gcc/testsuite/gcc.dg/lto/pr94271_1.c
new file mode 100644
index 00000000000..db9bc9df6db
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/lto/pr94271_1.c
@@ -0,0 +1,17 @@
+int aa;
+
+static inline int __attribute__ ((target_clones ("default", "avx512f")))
+fast_clamp ()
+{}
+
+void
+b ()
+{
+  aa = fast_clamp ();
+}
+
+int
+main ()
+{
+  return 0;
+}
diff --git a/gcc/testsuite/gcc.target/i386/pr81213-2.c b/gcc/testsuite/gcc.target/i386/pr81213-2.c
new file mode 100644
index 00000000000..a80622cb184
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/pr81213-2.c
@@ -0,0 +1,11 @@
+__attribute__((target_clones("avx","arch=slm","arch=core-avx2","default")))
+static int
+foo ()
+{
+  return 2;
+}
+
+int bar()
+{
+  return foo();
+}
diff --git a/gcc/testsuite/gcc.target/i386/pr81213.c b/gcc/testsuite/gcc.target/i386/pr81213.c
index 89c47529861..334838631d0 100644
--- a/gcc/testsuite/gcc.target/i386/pr81213.c
+++ b/gcc/testsuite/gcc.target/i386/pr81213.c
@@ -1,6 +1,9 @@
 /* PR ipa/81214.  */
-/* { dg-do compile } */
+/* { dg-do run } */
 /* { dg-require-ifunc "" } */
+/* { dg-additional-sources "pr81213-2.c" } */
+
+int bar();
 
 __attribute__((target_clones("avx","arch=slm","arch=core-avx2","default")))
 static int
@@ -11,7 +14,7 @@ foo ()
 
 int main()
 {
-  return foo();
+  return foo() + bar();
 }
 
 /* { dg-final { scan-assembler "\t.globl\tfoo" } } */
-- 
2.25.1

Re: [stage1] [PATCH] Make target_clones resolver fn static if possible.

[Jeff Law]

On Mon, 2020-03-23 at 16:09 +0100, Martin Liška wrote:
> From 1431a34f70128bdce59c94dad1d10f91673f63eb Mon Sep 17 00:00:00 2001
> From: Martin Liska <mliska@suse.cz>
> Date: Thu, 16 Jan 2020 10:38:41 +0100
> Subject: [PATCH] Make target_clones resolver fn static if possible.
> 
> gcc/ChangeLog:
> 
> 2020-03-17  Martin Liska  <mliska@suse.cz>
> 
> 	PR target/93274
> 	PR ipa/94271
> 	* config/i386/i386-features.c (make_resolver_func): Drop
> 	public flag for resolver.
> 	* config/rs6000/rs6000.c (make_resolver_func): Add comdat
> 	group for resolver and drop public flag if possible.
> 	* multiple_target.c (create_dispatcher_calls): Drop unique_name
> 	and resolution as we want to enable LTO privatization of the default
> 	symbol.
OK
jeff

Re: [stage1] [PATCH] Make target_clones resolver fn static if possible.

[Jakub Jelinek]

On Mon, Mar 23, 2020 at 04:09:52PM +0100, Martin Liška wrote:
> 2020-03-17  Martin Liska  <mliska@suse.cz>
> 
> 	PR target/93274	PR lto/94271
> 	* gcc.target/i386/pr81213-2.c: New test.
> 	* gcc.target/i386/pr81213.c: Add additional source.
> 	* gcc.dg/lto/pr94271_0.c: New test.
> 	* gcc.dg/lto/pr94271_1.c: New test.

I've noticed this test now has UNRESOLVED cases:
+UNRESOLVED: gcc.target/i386/pr81213.c scan-assembler \\t.globl\\tfoo
+UNRESOLVED: gcc.target/i386/pr81213.c scan-assembler foo.resolver:
+UNRESOLVED: gcc.target/i386/pr81213.c scan-assembler foo\\\\, @gnu_indirect_function

1) shall the test start with PR ipa/81214 reference when it is PR ipa/81213
test?
2) the UNRESOLVED cases can be fixed e.g. through:

--- gcc/testsuite/gcc.target/i386/pr81213.c.jj	2020-03-25 11:39:07.605865708 +0100
+++ gcc/testsuite/gcc.target/i386/pr81213.c	2020-03-26 10:13:23.616527400 +0100
@@ -1,6 +1,7 @@
 /* PR ipa/81214.  */
 /* { dg-do run } */
 /* { dg-require-ifunc "" } */
+/* { dg-options "-save-temps" } */
 /* { dg-additional-sources "pr81213-2.c" } */
 
 int bar();

3) but then one ends up with
FAIL: gcc.target/i386/pr81213.c scan-assembler \t.globl\tfoo

Do you want to change that to scan-assembler-not now that you don't want to
make foo public, or something else?

> --- a/gcc/testsuite/gcc.target/i386/pr81213.c
> +++ b/gcc/testsuite/gcc.target/i386/pr81213.c
> @@ -1,6 +1,9 @@
>  /* PR ipa/81214.  */
> -/* { dg-do compile } */
> +/* { dg-do run } */
>  /* { dg-require-ifunc "" } */
> +/* { dg-additional-sources "pr81213-2.c" } */
> +
> +int bar();
>  
>  __attribute__((target_clones("avx","arch=slm","arch=core-avx2","default")))
>  static int
> @@ -11,7 +14,7 @@ foo ()
>  
>  int main()
>  {
> -  return foo();
> +  return foo() + bar();
>  }
>  
>  /* { dg-final { scan-assembler "\t.globl\tfoo" } } */
> -- 
> 2.25.1
> 

	Jakub

Re: [stage1] [PATCH] Make target_clones resolver fn static if possible.

[Martin Liška]

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

On 3/26/20 10:23 AM, Jakub Jelinek wrote:
> On Mon, Mar 23, 2020 at 04:09:52PM +0100, Martin Liška wrote:
>> 2020-03-17  Martin Liska  <mliska@suse.cz>
>>
>> 	PR target/93274	PR lto/94271
>> 	* gcc.target/i386/pr81213-2.c: New test.
>> 	* gcc.target/i386/pr81213.c: Add additional source.
>> 	* gcc.dg/lto/pr94271_0.c: New test.
>> 	* gcc.dg/lto/pr94271_1.c: New test.
> 
> I've noticed this test now has UNRESOLVED cases:
> +UNRESOLVED: gcc.target/i386/pr81213.c scan-assembler \\t.globl\\tfoo
> +UNRESOLVED: gcc.target/i386/pr81213.c scan-assembler foo.resolver:
> +UNRESOLVED: gcc.target/i386/pr81213.c scan-assembler foo\\\\, @gnu_indirect_function
> 
> 1) shall the test start with PR ipa/81214 reference when it is PR ipa/81213
> test?

Probably yes, note that the test is relevant also to ipa/81214. So I'll add both PRs
references.

> 2) the UNRESOLVED cases can be fixed e.g. through:
> 
> --- gcc/testsuite/gcc.target/i386/pr81213.c.jj	2020-03-25 11:39:07.605865708 +0100
> +++ gcc/testsuite/gcc.target/i386/pr81213.c	2020-03-26 10:13:23.616527400 +0100
> @@ -1,6 +1,7 @@
>   /* PR ipa/81214.  */
>   /* { dg-do run } */
>   /* { dg-require-ifunc "" } */
> +/* { dg-options "-save-temps" } */
>   /* { dg-additional-sources "pr81213-2.c" } */
>   
>   int bar();
> 
> 3) but then one ends up with
> FAIL: gcc.target/i386/pr81213.c scan-assembler \t.globl\tfoo

The asm scan does not make sense now as it's a run-time test
and we would see a linker error in case of 2 .globl foo symbols.

I'm suggesting a patch.
Martin

> 
> Do you want to change that to scan-assembler-not now that you don't want to
> make foo public, or something else?
> 
>> --- a/gcc/testsuite/gcc.target/i386/pr81213.c
>> +++ b/gcc/testsuite/gcc.target/i386/pr81213.c
>> @@ -1,6 +1,9 @@
>>   /* PR ipa/81214.  */
>> -/* { dg-do compile } */
>> +/* { dg-do run } */
>>   /* { dg-require-ifunc "" } */
>> +/* { dg-additional-sources "pr81213-2.c" } */
>> +
>> +int bar();
>>   
>>   __attribute__((target_clones("avx","arch=slm","arch=core-avx2","default")))
>>   static int
>> @@ -11,7 +14,7 @@ foo ()
>>   
>>   int main()
>>   {
>> -  return foo();
>> +  return foo() + bar();
>>   }
>>   
>>   /* { dg-final { scan-assembler "\t.globl\tfoo" } } */
>> -- 
>> 2.25.1
>>
> 
> 	Jakub
> 


[-- Attachment #2: 0001-Fix-UNRESOLVED-test-case.patch --]
[-- Type: text/x-patch, Size: 1038 bytes --]

From 6a5c17037c0a20bc687a0c08234928959f441971 Mon Sep 17 00:00:00 2001
From: Martin Liska <mliska@suse.cz>
Date: Thu, 26 Mar 2020 10:45:50 +0100
Subject: [PATCH] Fix UNRESOLVED test-case.

gcc/testsuite/ChangeLog:

2020-03-26  Martin Liska  <mliska@suse.cz>

	* gcc.target/i386/pr81213.c: Do not scan assembler
	and add one missing PR entry.
---
 gcc/testsuite/gcc.target/i386/pr81213.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/gcc/testsuite/gcc.target/i386/pr81213.c b/gcc/testsuite/gcc.target/i386/pr81213.c
index 334838631d0..6194e9ecccd 100644
--- a/gcc/testsuite/gcc.target/i386/pr81213.c
+++ b/gcc/testsuite/gcc.target/i386/pr81213.c
@@ -1,3 +1,4 @@
+/* PR ipa/81213.  */
 /* PR ipa/81214.  */
 /* { dg-do run } */
 /* { dg-require-ifunc "" } */
@@ -16,7 +17,3 @@ int main()
 {
   return foo() + bar();
 }
-
-/* { dg-final { scan-assembler "\t.globl\tfoo" } } */
-/* { dg-final { scan-assembler "foo.resolver:" } } */
-/* { dg-final { scan-assembler "foo\\, @gnu_indirect_function" } } */
-- 
2.25.1

Re: [stage1] [PATCH] Make target_clones resolver fn static if possible.

[Jakub Jelinek]

On Thu, Mar 26, 2020 at 10:46:48AM +0100, Martin Liška wrote:
> 2020-03-26  Martin Liska  <mliska@suse.cz>
> 
> 	* gcc.target/i386/pr81213.c: Do not scan assembler
> 	and add one missing PR entry.

Ok.

> ---
>  gcc/testsuite/gcc.target/i386/pr81213.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/gcc/testsuite/gcc.target/i386/pr81213.c b/gcc/testsuite/gcc.target/i386/pr81213.c
> index 334838631d0..6194e9ecccd 100644
> --- a/gcc/testsuite/gcc.target/i386/pr81213.c
> +++ b/gcc/testsuite/gcc.target/i386/pr81213.c
> @@ -1,3 +1,4 @@
> +/* PR ipa/81213.  */
>  /* PR ipa/81214.  */
>  /* { dg-do run } */
>  /* { dg-require-ifunc "" } */
> @@ -16,7 +17,3 @@ int main()
>  {
>    return foo() + bar();
>  }
> -
> -/* { dg-final { scan-assembler "\t.globl\tfoo" } } */
> -/* { dg-final { scan-assembler "foo.resolver:" } } */
> -/* { dg-final { scan-assembler "foo\\, @gnu_indirect_function" } } */
> -- 
> 2.25.1
> 


	Jakub

Re: [PATCH] Make target_clones resolver fn static.

[Jeff Law]

On Tue, 2020-01-21 at 13:48 +0100, Martin Liška wrote:
> From a3faaced989869867671ceadd89b56fabde225ff Mon Sep 17 00:00:00 2001
> From: Martin Liska <mliska@suse.cz>
> Date: Thu, 16 Jan 2020 10:38:41 +0100
> Subject: [PATCH] Make target_clones resolver fn static.
> 
> gcc/ChangeLog:
> 
> 2020-01-17  Martin Liska  <mliska@suse.cz>
> 
> 	PR target/93274
> 	* config/i386/i386-features.c (make_resolver_func):
> 	Align the code with ppc64 target implementaion.
> 	We do not need to have gnu_indirect_function
> 	as a global function.  Drop TREE_PUBLIC on
> 	ifunc symbol.
> 	* config/rs6000/rs6000.c (make_resolver_func): Drop
> 	TREE_PUBLIC on ifunc symbol.
> 
> gcc/testsuite/ChangeLog:
> 
> 2020-01-17  Martin Liska  <mliska@suse.cz>
> 
> 	PR target/93274
> 	* gcc.target/i386/pr81213.c: Adjust to not expect
> 	a global unique name.
> 	* gcc.target/i386/pr81213-2.c: New test.
Not strictly a regression, but given the codegen impact, I think this
should go in.  OK

jeff

Re: [PATCH] Make target_clones resolver fn static.

[Martin Liška]

On 1/26/20 6:35 PM, Jeff Law wrote:
> On Tue, 2020-01-21 at 13:48 +0100, Martin Liška wrote:
>>  From a3faaced989869867671ceadd89b56fabde225ff Mon Sep 17 00:00:00 2001
>> From: Martin Liska <mliska@suse.cz>
>> Date: Thu, 16 Jan 2020 10:38:41 +0100
>> Subject: [PATCH] Make target_clones resolver fn static.
>>
>> gcc/ChangeLog:
>>
>> 2020-01-17  Martin Liska  <mliska@suse.cz>
>>
>> 	PR target/93274
>> 	* config/i386/i386-features.c (make_resolver_func):
>> 	Align the code with ppc64 target implementaion.
>> 	We do not need to have gnu_indirect_function
>> 	as a global function.  Drop TREE_PUBLIC on
>> 	ifunc symbol.
>> 	* config/rs6000/rs6000.c (make_resolver_func): Drop
>> 	TREE_PUBLIC on ifunc symbol.
>>
>> gcc/testsuite/ChangeLog:
>>
>> 2020-01-17  Martin Liska  <mliska@suse.cz>
>>
>> 	PR target/93274
>> 	* gcc.target/i386/pr81213.c: Adjust to not expect
>> 	a global unique name.
>> 	* gcc.target/i386/pr81213-2.c: New test.
> Not strictly a regression, but given the codegen impact, I think this
> should go in.  OK

Thanks for the approval, but I'm going to install only the first part and leave
the second part for GCC 11.

Martin

> 
> jeff
>