On Tue, Aug 13, 2019 at 4:22 AM luoxhu <luo...@linux.ibm.com> wrote: > > Hi Richard, > > On 2019/8/12 16:51, Richard Biener wrote: > > On Mon, Aug 12, 2019 at 8:50 AM luoxhu <luo...@linux.ibm.com> wrote: > >> > >> Hi Richard, > >> Thanks for your comments, updated the v2 patch as below: > >> 1. Define and use builtin_with_linkage_p. > >> 2. Add comments. > >> 3. Add a testcase. > >> > >> In LTO mode, if static library and dynamic library contains same > >> function and both libraries are passed as arguments, linker will link > >> the function in dynamic library no matter the sequence. This patch > >> will output LTO symbol node as UNDEF if BUILT_IN_NORMAL function FNDECL > >> is a math function, then the function in static library will be linked > >> first if its sequence is ahead of the dynamic library. > > > > Comments below > > > >> gcc/ChangeLog > >> > >> 2019-08-12 Xiong Hu Luo <luo...@linux.ibm.com> > >> > >> PR lto/91287 > >> * builtins.c (builtin_with_linkage_p): New function. > >> * builtins.h (builtin_with_linkage_p): New function. > >> * symtab.c (write_symbol): Use builtin_with_linkage_p. > >> * lto-streamer-out.c (symtab_node::output_to_lto_symbol_table_p): > >> Likewise. > >> > >> gcc/testsuite/ChangeLog > >> > >> 2019-08-12 Xiong Hu Luo <luo...@linux.ibm.com> > >> > >> PR lto/91287 > >> * gcc.dg/pr91287.c: New testcase. > >> --- > >> gcc/builtins.c | 89 ++++++++++++++++++++++++++++++++++ > >> gcc/builtins.h | 2 + > >> gcc/lto-streamer-out.c | 4 +- > >> gcc/symtab.c | 13 ++++- > >> gcc/testsuite/gcc.dg/pr91287.c | 40 +++++++++++++++ > >> 5 files changed, 145 insertions(+), 3 deletions(-) > >> create mode 100644 gcc/testsuite/gcc.dg/pr91287.c > >> > >> diff --git a/gcc/builtins.c b/gcc/builtins.c > >> index 695a9d191af..f4dea941a27 100644 > >> --- a/gcc/builtins.c > >> +++ b/gcc/builtins.c > >> @@ -11244,3 +11244,92 @@ target_char_cst_p (tree t, char *p) > >> *p = (char)tree_to_uhwi (t); > >> return true; > >> } > >> + > >> +/* Return true if DECL is a specified builtin math function. These > >> functions > >> + should have symbol in symbol table to provide linkage with faster > >> version of > >> + libraries. */ > > > > The comment should read like > > > > /* Return true if the builtin DECL is implemented in a standard > > library. Otherwise > > returns false which doesn't guarantee it is not (thus the list of > > handled builtins > > below may be incomplete). */ > > > >> +bool > >> +builtin_with_linkage_p (tree decl) > >> +{ > >> + if (!decl) > >> + return false; > > > > Omit this check please. > > > >> + if (DECL_BUILT_IN_CLASS (decl) == BUILT_IN_NORMAL) > >> + switch (DECL_FUNCTION_CODE (decl)) > >> + { > >> + CASE_FLT_FN (BUILT_IN_ACOS): > >> + CASE_FLT_FN (BUILT_IN_ACOSH): > >> + CASE_FLT_FN (BUILT_IN_ASIN): > >> + CASE_FLT_FN (BUILT_IN_ASINH): > >> + CASE_FLT_FN (BUILT_IN_ATAN): > >> + CASE_FLT_FN (BUILT_IN_ATANH): > >> + CASE_FLT_FN (BUILT_IN_ATAN2): > >> + CASE_FLT_FN (BUILT_IN_CBRT): > >> + CASE_FLT_FN (BUILT_IN_CEIL): > >> + CASE_FLT_FN_FLOATN_NX (BUILT_IN_CEIL): > >> + CASE_FLT_FN (BUILT_IN_COPYSIGN): > >> + CASE_FLT_FN_FLOATN_NX (BUILT_IN_COPYSIGN): > >> + CASE_FLT_FN (BUILT_IN_COS): > >> + CASE_FLT_FN (BUILT_IN_COSH): > >> + CASE_FLT_FN (BUILT_IN_ERF): > >> + CASE_FLT_FN (BUILT_IN_ERFC): > >> + CASE_FLT_FN (BUILT_IN_EXP): > >> + CASE_FLT_FN (BUILT_IN_EXP2): > >> + CASE_FLT_FN (BUILT_IN_EXPM1): > >> + CASE_FLT_FN (BUILT_IN_FABS): > >> + CASE_FLT_FN_FLOATN_NX (BUILT_IN_FABS): > >> + CASE_FLT_FN (BUILT_IN_FDIM): > >> + CASE_FLT_FN (BUILT_IN_FLOOR): > >> + CASE_FLT_FN_FLOATN_NX (BUILT_IN_FLOOR): > >> + CASE_FLT_FN (BUILT_IN_FMA): > >> + CASE_FLT_FN_FLOATN_NX (BUILT_IN_FMA): > >> + CASE_FLT_FN (BUILT_IN_FMAX): > >> + CASE_FLT_FN_FLOATN_NX (BUILT_IN_FMAX): > >> + CASE_FLT_FN (BUILT_IN_FMIN): > >> + CASE_FLT_FN_FLOATN_NX (BUILT_IN_FMIN): > >> + CASE_FLT_FN (BUILT_IN_FMOD): > >> + CASE_FLT_FN (BUILT_IN_FREXP): > >> + CASE_FLT_FN (BUILT_IN_HYPOT): > >> + CASE_FLT_FN (BUILT_IN_ILOGB): > >> + CASE_FLT_FN (BUILT_IN_LDEXP): > >> + CASE_FLT_FN (BUILT_IN_LGAMMA): > >> + CASE_FLT_FN (BUILT_IN_LLRINT): > >> + CASE_FLT_FN (BUILT_IN_LLROUND): > >> + CASE_FLT_FN (BUILT_IN_LOG): > >> + CASE_FLT_FN (BUILT_IN_LOG10): > >> + CASE_FLT_FN (BUILT_IN_LOG1P): > >> + CASE_FLT_FN (BUILT_IN_LOG2): > >> + CASE_FLT_FN (BUILT_IN_LOGB): > >> + CASE_FLT_FN (BUILT_IN_LRINT): > >> + CASE_FLT_FN (BUILT_IN_LROUND): > >> + CASE_FLT_FN (BUILT_IN_MODF): > >> + CASE_FLT_FN (BUILT_IN_NAN): > >> + CASE_FLT_FN (BUILT_IN_NEARBYINT): > >> + CASE_FLT_FN_FLOATN_NX (BUILT_IN_NEARBYINT): > >> + CASE_FLT_FN (BUILT_IN_NEXTAFTER): > >> + CASE_FLT_FN (BUILT_IN_NEXTTOWARD): > >> + CASE_FLT_FN (BUILT_IN_POW): > >> + CASE_FLT_FN (BUILT_IN_REMAINDER): > >> + CASE_FLT_FN (BUILT_IN_REMQUO): > >> + CASE_FLT_FN (BUILT_IN_RINT): > >> + CASE_FLT_FN_FLOATN_NX (BUILT_IN_RINT): > >> + CASE_FLT_FN (BUILT_IN_ROUND): > >> + CASE_FLT_FN_FLOATN_NX (BUILT_IN_ROUND): > >> + CASE_FLT_FN (BUILT_IN_SCALBLN): > >> + CASE_FLT_FN (BUILT_IN_SCALBN): > >> + CASE_FLT_FN (BUILT_IN_SIN): > >> + CASE_FLT_FN (BUILT_IN_SINH): > >> + CASE_FLT_FN (BUILT_IN_SINCOS): > >> + CASE_FLT_FN (BUILT_IN_SQRT): > >> + CASE_FLT_FN_FLOATN_NX (BUILT_IN_SQRT): > >> + CASE_FLT_FN (BUILT_IN_TAN): > >> + CASE_FLT_FN (BUILT_IN_TANH): > >> + CASE_FLT_FN (BUILT_IN_TGAMMA): > >> + CASE_FLT_FN (BUILT_IN_TRUNC): > >> + CASE_FLT_FN_FLOATN_NX (BUILT_IN_TRUNC): > >> + return true; > >> + default: > >> + break; > >> + } > >> + return false; > >> +} > >> diff --git a/gcc/builtins.h b/gcc/builtins.h > >> index 1ffb491d785..91cbd81be48 100644 > >> --- a/gcc/builtins.h > >> +++ b/gcc/builtins.h > >> @@ -151,4 +151,6 @@ extern internal_fn replacement_internal_fn (gcall *); > >> extern void warn_string_no_nul (location_t, const char *, tree, tree); > >> extern tree unterminated_array (tree, tree * = NULL, bool * = NULL); > >> > >> +extern bool builtin_with_linkage_p (tree); > > > > no vertical space above this line. > > > >> + > >> #endif /* GCC_BUILTINS_H */ > >> diff --git a/gcc/lto-streamer-out.c b/gcc/lto-streamer-out.c > >> index 47a9143ae26..73bf45810f0 100644 > >> --- a/gcc/lto-streamer-out.c > >> +++ b/gcc/lto-streamer-out.c > >> @@ -2644,7 +2644,9 @@ write_symbol (struct streamer_tree_cache_d *cache, > >> > >> gcc_checking_assert (TREE_PUBLIC (t) > >> && (TREE_CODE (t) != FUNCTION_DECL > >> - || !fndecl_built_in_p (t)) > >> + || cgraph_node::get (t)->definition > >> + || !fndecl_built_in_p (t, BUILT_IN_NORMAL) > >> + || builtin_with_linkage_p (t)) > >> && !DECL_ABSTRACT_P (t) > >> && (!VAR_P (t) || !DECL_HARD_REGISTER (t))); > > > > Please remove the whole assert - both callers are guarded by > > symtab_node::output_to_lto_symbol_table_p which means it is redundant > > and fragile to keep in-sync. > > > >> diff --git a/gcc/symtab.c b/gcc/symtab.c > >> index 63e2820eb93..e806afdede4 100644 > >> --- a/gcc/symtab.c > >> +++ b/gcc/symtab.c > >> @@ -2377,8 +2377,17 @@ symtab_node::output_to_lto_symbol_table_p (void) > >> return false; > >> /* FIXME: Builtins corresponding to real functions probably should have > >> symbol table entries. */ > > > > Remove this comment > > > >> - if (TREE_CODE (decl) == FUNCTION_DECL && fndecl_built_in_p (decl)) > >> - return false; > >> + if (TREE_CODE (decl) == FUNCTION_DECL && !definition > >> + && fndecl_built_in_p (decl)) > >> + { > >> + /* Math functions may have faster version in static library, output > >> the > >> + symbol for linkage. Functions have implementation in libgcc will > >> be > >> + excluded as the symbol name may mismatch. */ > > > > /* Builtins like those for most math functions have actual > > implementations in > > libraries so make sure to output references into the symbol table to > > make > > those libraries referenced. Note this is incomplete handling for now > > and > > only covers math functions. */ > > > >> + if (builtin_with_linkage_p (decl)) > >> + return true; > >> + else > >> + return false; > >> + } > >> > >> /* We have real symbol that should be in symbol table. However try to > >> trim > >> down the refernces to libraries bit more because linker will > >> otherwise > >> diff --git a/gcc/testsuite/gcc.dg/pr91287.c > >> b/gcc/testsuite/gcc.dg/pr91287.c > >> new file mode 100644 > >> index 00000000000..c816e0537aa > >> --- /dev/null > >> +++ b/gcc/testsuite/gcc.dg/pr91287.c > >> @@ -0,0 +1,40 @@ > >> +/* { dg-do assemble } */ > >> +/* { dg-options "-O2" } */ > > > > You don't use -flto here so the testcase doesn't exercise any of the patched > > code. Does it work when you add -flto here? That is, do > > scan-symbol[-not] properly use gcc-nm or the linker plugin? > > -flto is needed here to check this patch correctness, my mistake here, > thanks for catching. atan2 will exists in pr91287.o even without lto as > pr91287.o has the instruction "bl atan2".
Sure, that's expected. It is expected that without the patch but with -flto the symbol doesn't appear. Note that to fail we rely on -fno-fat-lto-objects and linker plugin availability. So you should add /* { dg-require-linker-plugin "" } */ after /* { dg-do assemble } */ and use -O2 -flto -fno-fat-lto-objects -fuse-linker-plugin the other issue is that scan-symbol invokes 'nm' without any plugin arguments so we rely on plugin auto-loading and a system-installed plugin. We could use built gcc-nm here if present but would need to pass an appropriate -B argument, see how we find xg++ in g++.exp and scan-symbol-common in gcc-dg.exp. It might be better / easier to move the testcase to gcc.dg/lto/ but lto.exp might not support scan-symbol ... So... I think the patch is ok as-is if you omit the testcase which needs more work to be useful (and not sure if worth all the trouble). Thanks, Richard. > After adding -flto the case also works as symbol is written to pr91287.o. > PS: update other changes in patch attached. > Xionghu > > > > >> +#include <stdio.h> > >> +#include <string.h> > >> +#include <math.h> > >> +#include <stdlib.h> > >> + > >> +double __attribute__((noinline)) zowie (double x, double y, double z) > >> +{ > >> + x = __builtin_clz(x); > >> + return atan2 (x * y, z); > >> +} > >> + > >> +double __attribute__((noinline)) rand_finite_double (void) > >> +{ > >> + union { > >> + double d; > >> + unsigned char uc[sizeof(double)]; > >> + } u; > >> + do { > >> + for (unsigned i = 0; i < sizeof u.uc; i++) { > >> + u.uc[i] = (unsigned char) rand(); > >> + } > >> + } while (!isfinite(u.d)); > >> + return u.d; > >> +} > >> + > >> + > >> +int main () > >> +{ > >> + char str[100]; > >> + strcpy(str, "test\n"); > >> + double a = rand_finite_double (); > >> + printf ("%lf, %s\n", zowie (a, 4.5, 2.2), str); > >> + return 0; > >> +} > >> + > >> +/* { dg-final { scan-symbol "atan2" } } */ > >> +/* { dg-final { scan-symbol-not "__builtin_clz" } } */ > >> -- > >> 2.21.0.777.g83232e3864 > >> > >