Make target_clones resolver fn static.

Message ID eb85e847-a0d5-ed3a-534d-f67738c8281f@suse.cz
State New
Headers show
Series
  • Make target_clones resolver fn static.
Related show

Commit Message

Martin Liška Jan. 17, 2020, 9:25 a.m.
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(-)

Comments

Richard Biener Jan. 20, 2020, 10:25 a.m. | #1
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(-)

>

>
H.J. Lu Jan. 20, 2020, 1:23 p.m. | #2
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.
Alexander Monakov Jan. 20, 2020, 1:36 p.m. | #3
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
H.J. Lu Jan. 20, 2020, 1:40 p.m. | #4
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.
Alexander Monakov Jan. 20, 2020, 2:16 p.m. | #5
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
H.J. Lu Jan. 20, 2020, 2:19 p.m. | #6
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.
Alexander Monakov Jan. 20, 2020, 2:41 p.m. | #7
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
H.J. Lu Jan. 20, 2020, 2:46 p.m. | #8
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.
Richard Biener Jan. 20, 2020, 2:52 p.m. | #9
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.
Martin Liška Jan. 21, 2020, 12:48 p.m. | #10
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.
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
Richard Biener Jan. 23, 2020, 1:09 p.m. | #11
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.

>
Martin Liška Jan. 23, 2020, 1:43 p.m. | #12
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.

>>
Alexander Monakov Jan. 23, 2020, 1:52 p.m. | #13
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
Jeff Law Jan. 26, 2020, 5:35 p.m. | #14
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
Martin Liška Jan. 27, 2020, 9:46 a.m. | #15
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

>
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
Martin Liška Jan. 27, 2020, 9:47 a.m. | #16
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

>

Patch

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" } } */