| Message ID | eb85e847-a0d5-ed3a-534d-f67738c8281f@suse.cz |
|---|---|
| State | New |
| Headers | show |
| Series | Make target_clones resolver fn static. | expand |
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(-) > >
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(-) > > > >
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
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.
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
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.
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
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.
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.
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.
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. >
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. >>
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
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
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 >
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 >
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" } } */