Message ID | 546DDFB4.9050302@codesourcery.com |
---|---|
State | New |
Headers | show |
On 11/20/14 05:33, Bernd Schmidt wrote: > Now that I've managed to put together and test all the submitted OpenACC > patches I found there was one piece missing. The problem is that omp-low > on the host likes to generate function names like "_main._omp_fn". On > ptx, the dot is not allowed in identifiers, so we have to rewrite this > to use a dollar sign. > > The patch below does this at the lto-read stage. Bootstrapped on > x86_64-linux, ok if testing is successful? Was expecting Richi or Honza to review this... They certainly know the LTO bits far better than I. Or have you ultimately addressed this issue some other way? jeff
On Thu, Jan 15, 2015 at 7:35 PM, Jeff Law <law@redhat.com> wrote: > On 11/20/14 05:33, Bernd Schmidt wrote: >> >> Now that I've managed to put together and test all the submitted OpenACC >> patches I found there was one piece missing. The problem is that omp-low >> on the host likes to generate function names like "_main._omp_fn". On >> ptx, the dot is not allowed in identifiers, so we have to rewrite this >> to use a dollar sign. >> >> The patch below does this at the lto-read stage. Bootstrapped on >> x86_64-linux, ok if testing is successful? > > Was expecting Richi or Honza to review this... They certainly know the LTO > bits far better than I. Or have you ultimately addressed this issue some > other way? I don't like the validize_symbol_for_target thing - you create proper symbols elsewhere from the start - what's left to fix? IMHO we should have a single helper somewhere for concatenating with some separator. Richard. > jeff >
On Fri, Jan 16, 2015 at 11:45:40AM +0100, Richard Biener wrote: > On Thu, Jan 15, 2015 at 7:35 PM, Jeff Law <law@redhat.com> wrote: > > On 11/20/14 05:33, Bernd Schmidt wrote: > >> > >> Now that I've managed to put together and test all the submitted OpenACC > >> patches I found there was one piece missing. The problem is that omp-low > >> on the host likes to generate function names like "_main._omp_fn". On > >> ptx, the dot is not allowed in identifiers, so we have to rewrite this > >> to use a dollar sign. > >> > >> The patch below does this at the lto-read stage. Bootstrapped on > >> x86_64-linux, ok if testing is successful? > > > > Was expecting Richi or Honza to review this... They certainly know the LTO > > bits far better than I. Or have you ultimately addressed this issue some > > other way? > > I don't like the validize_symbol_for_target thing - you create proper > symbols elsewhere from the start - what's left to fix? IMHO we should > have a single helper somewhere for concatenating with some separator. The problem is that the host compiler does not and cannot know what characters are valid or invalid on the offloading target (and, especially if you have multiple of them, as we do now, where some of the .$_ characters are valid and others are not). The only universally available character is _ I assume, but that one has the problem that users can type that in their code and clash with the internal symbols. And, if we do not want to use _ from the beginning, what do you suggest otherwise? Also note that normally the same functions are emitted for both host and offloading target, you don't have different symbol names for host and different for what you want to stream to the offloading LTO sections. So I think something like Bernd's patch is the way to go. Another alternative is to pick up some completely different character not valid in any identifiers, and do two transformations of the names. When streaming offloading LTO from the host compiler, mangle the $ and . characters in names to some other character, say %, and then when streaming the offloading LTO into the offloading compiler, remap the % (or whatever) character to the best internal symbol character there - ., $ or _ (in that order?). But Bernd's patch looks simpler than that. Jakub
On Wed, Feb 4, 2015 at 11:38 AM, Jakub Jelinek <jakub@redhat.com> wrote: > On Fri, Jan 16, 2015 at 11:45:40AM +0100, Richard Biener wrote: >> On Thu, Jan 15, 2015 at 7:35 PM, Jeff Law <law@redhat.com> wrote: >> > On 11/20/14 05:33, Bernd Schmidt wrote: >> >> >> >> Now that I've managed to put together and test all the submitted OpenACC >> >> patches I found there was one piece missing. The problem is that omp-low >> >> on the host likes to generate function names like "_main._omp_fn". On >> >> ptx, the dot is not allowed in identifiers, so we have to rewrite this >> >> to use a dollar sign. >> >> >> >> The patch below does this at the lto-read stage. Bootstrapped on >> >> x86_64-linux, ok if testing is successful? >> > >> > Was expecting Richi or Honza to review this... They certainly know the LTO >> > bits far better than I. Or have you ultimately addressed this issue some >> > other way? >> >> I don't like the validize_symbol_for_target thing - you create proper >> symbols elsewhere from the start - what's left to fix? IMHO we should >> have a single helper somewhere for concatenating with some separator. > > The problem is that the host compiler does not and cannot know what > characters are valid or invalid on the offloading target (and, especially if > you have multiple of them, as we do now, where some of the .$_ characters > are valid and others are not). > The only universally available character is _ I assume, but that one has the > problem that users can type that in their code and clash with the internal > symbols. And, if we do not want to use _ from the beginning, what do you > suggest otherwise? Also note that normally the same functions are emitted > for both host and offloading target, you don't have different symbol names > for host and different for what you want to stream to the offloading LTO > sections. > > So I think something like Bernd's patch is the way to go. Another > alternative is to pick up some completely different character not valid > in any identifiers, and do two transformations of the names. When streaming > offloading LTO from the host compiler, mangle the $ and . characters in > names to some other character, say %, and then when streaming the offloading > LTO into the offloading compiler, remap the % (or whatever) character to > the best internal symbol character there - ., $ or _ (in that order?). > But Bernd's patch looks simpler than that. Bernds patch is ok with the unrelated gcc.c hunk removed. Thanks, Richard. > Jakub
On 02/10/2015 11:34 AM, Richard Biener wrote:
> Bernds patch is ok with the unrelated gcc.c hunk removed.
Thanks. I'm assuming this is for the next stage 1?
Bernd
On Tue, Feb 10, 2015 at 01:06:15PM +0100, Bernd Schmidt wrote: > On 02/10/2015 11:34 AM, Richard Biener wrote: > > >Bernds patch is ok with the unrelated gcc.c hunk removed. > > Thanks. I'm assuming this is for the next stage 1? No, for current trunk, we don't want to ship with offloading to nvptx completely broken, which is my understanding of the current state. There is another patch that has been approved already, nvptx offloading patches [1/n] (and I think hasn't been committed yet), then there is the va_list issue where I'm afraid your patch will break normal LTO badly, it would be nice to discuss that. And finally for the enum machine_mode translation, I've started working on some patch, but didn't get too far yet, interrupted by other tasks. Jakub
commit 26b41de43c6db6e2368a9511c589c433b1e49c96 Author: Bernd Schmidt <bernds@codesourcery.com> Date: Wed Nov 19 21:47:59 2014 +0100 Renaming for invalid symbols when reading LTO. * cgraph.h (clone_function_name_1): Declare. * cgraphclones.c (clone_function_name_1): New function. (clone_function_name): Use it. * lto-partition.c: Include "stringpool.h". (must_not_rename, maybe_rewrite_identifier, validize_symbol_for_target): New static functions. (privatize_symbol_name): Use must_not_rename. (promote_symbol): Call validize_symbol_for_target. (lto_promote_cross_file_statics): Likewise. (lto_promote_statics_nonwpa): Likewise. diff --git a/gcc/cgraph.h b/gcc/cgraph.h index a5c5f56..7be6413 100644 --- a/gcc/cgraph.h +++ b/gcc/cgraph.h @@ -2150,6 +2150,7 @@ basic_block init_lowered_empty_function (tree, bool); /* In cgraphclones.c */ +tree clone_function_name_1 (const char *, const char *); tree clone_function_name (tree decl, const char *); void tree_function_versioning (tree, tree, vec<ipa_replace_map *, va_gc> *, diff --git a/gcc/cgraphclones.c b/gcc/cgraphclones.c index 086dd92..1b7d8d2 100644 --- a/gcc/cgraphclones.c +++ b/gcc/cgraphclones.c @@ -506,19 +506,19 @@ cgraph_node::create_clone (tree decl, gcov_type gcov_count, int freq, return new_node; } -/* Return a new assembler name for a clone of DECL with SUFFIX. */ - static GTY(()) unsigned int clone_fn_id_num; +/* Return a new assembler name for a clone with SUFFIX of a decl named + NAME. */ + tree -clone_function_name (tree decl, const char *suffix) +clone_function_name_1 (const char *name, const char *suffix) { - tree name = DECL_ASSEMBLER_NAME (decl); - size_t len = IDENTIFIER_LENGTH (name); + size_t len = strlen (name); char *tmp_name, *prefix; prefix = XALLOCAVEC (char, len + strlen (suffix) + 2); - memcpy (prefix, IDENTIFIER_POINTER (name), len); + memcpy (prefix, name, len); strcpy (prefix + len + 1, suffix); #ifndef NO_DOT_IN_LABEL prefix[len] = '.'; @@ -531,6 +531,16 @@ clone_function_name (tree decl, const char *suffix) return get_identifier (tmp_name); } +/* Return a new assembler name for a clone of DECL with SUFFIX. */ + +tree +clone_function_name (tree decl, const char *suffix) +{ + tree name = DECL_ASSEMBLER_NAME (decl); + return clone_function_name_1 (IDENTIFIER_POINTER (name), suffix); +} + + /* Create callgraph node clone with new declaration. The actual body will be copied later at compilation stage. diff --git a/gcc/gcc.c b/gcc/gcc.c index 80dc87c..c49401b 100644 --- a/gcc/gcc.c +++ b/gcc/gcc.c @@ -4238,14 +4238,14 @@ process_command (unsigned int decoded_options_count, } gcc_assert (!IS_ABSOLUTE_PATH (tooldir_base_prefix)); - tooldir_prefix2 = concat (tooldir_base_prefix, spec_host_machine, + tooldir_prefix2 = concat (tooldir_base_prefix, spec_machine, dir_separator_str, NULL); /* Look for tools relative to the location from which the driver is running, or, if that is not available, the configured prefix. */ tooldir_prefix = concat (gcc_exec_prefix ? gcc_exec_prefix : standard_exec_prefix, - spec_host_machine, dir_separator_str, spec_version, + spec_machine, dir_separator_str, spec_version, accel_dir_suffix, dir_separator_str, tooldir_prefix2, NULL); free (tooldir_prefix2); diff --git a/gcc/lto/lto-partition.c b/gcc/lto/lto-partition.c index 65f0582..ac10c90 100644 --- a/gcc/lto/lto-partition.c +++ b/gcc/lto/lto-partition.c @@ -49,6 +49,7 @@ along with GCC; see the file COPYING3. If not see #include "ipa-inline.h" #include "ipa-utils.h" #include "lto-partition.h" +#include "stringpool.h" vec<ltrans_partition> ltrans_partitions; @@ -775,21 +776,12 @@ lto_balanced_map (int n_lto_partitions) free (order); } -/* Mangle NODE symbol name into a local name. - This is necessary to do - 1) if two or more static vars of same assembler name - are merged into single ltrans unit. - 2) if prevoiusly static var was promoted hidden to avoid possible conflict - with symbols defined out of the LTO world. -*/ +/* Return true if we must not change the name of the NODE. The name as + extracted from the corresponding decl should be passed in NAME. */ static bool -privatize_symbol_name (symtab_node *node) +must_not_rename (symtab_node *node, const char *name) { - tree decl = node->decl; - const char *name = IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (decl)); - cgraph_node *cnode; - /* Our renaming machinery do not handle more than one change of assembler name. We should not need more than one anyway. */ if (node->lto_file_data @@ -797,9 +789,9 @@ privatize_symbol_name (symtab_node *node) { if (symtab->dump_file) fprintf (symtab->dump_file, - "Not privatizing symbol name: %s. It privatized already.\n", - name); - return false; + "Not privatizing symbol name: %s. It privatized already.\n", + name); + return true; } /* Avoid mangling of already mangled clones. ??? should have a flag whether a symbol has a 'private' name already, @@ -809,12 +801,95 @@ privatize_symbol_name (symtab_node *node) { if (symtab->dump_file) fprintf (symtab->dump_file, - "Not privatizing symbol name: %s. Has unique name.\n", - name); - return false; + "Not privatizing symbol name: %s. Has unique name.\n", + name); + return true; + } + return false; +} + +/* If we are an offload compiler, we may have to rewrite symbols to be + valid on this target. Return either PTR or a modified version of it. */ + +static const char * +maybe_rewrite_identifier (const char *ptr) +{ +#if defined ACCEL_COMPILER && (defined NO_DOT_IN_LABEL || defined NO_DOLLAR_IN_LABEL) +#ifndef NO_DOT_IN_LABEL + char valid = '.'; + const char reject[] = "$"; +#elif !defined NO_DOLLAR_IN_LABEL + char valid = '$'; + const char reject[] = "."; +#else + char valid = '_'; + const char reject[] = ".$"; +#endif + + char *copy = NULL; + const char *match = ptr; + for (;;) + { + size_t off = strcspn (match, reject); + if (match[off] == '\0') + break; + if (copy == NULL) + { + copy = xstrdup (ptr); + match = copy; + } + copy[off] = valid; + } + return match; +#else + return ptr; +#endif +} + +/* Ensure that the symbol in NODE is valid for the target, and if not, + rewrite it. */ + +static void +validize_symbol_for_target (symtab_node *node) +{ + tree decl = node->decl; + const char *name = IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (decl)); + + if (must_not_rename (node, name)) + return; + + const char *name2 = maybe_rewrite_identifier (name); + if (name2 != name) + { + symtab->change_decl_assembler_name (decl, get_identifier (name2)); + if (node->lto_file_data) + lto_record_renamed_decl (node->lto_file_data, name, + IDENTIFIER_POINTER + (DECL_ASSEMBLER_NAME (decl))); } +} + +/* Mangle NODE symbol name into a local name. + This is necessary to do + 1) if two or more static vars of same assembler name + are merged into single ltrans unit. + 2) if previously static var was promoted hidden to avoid possible conflict + with symbols defined out of the LTO world. */ + +static bool +privatize_symbol_name (symtab_node *node) +{ + tree decl = node->decl; + const char *name = IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (decl)); + cgraph_node *cnode; + + if (must_not_rename (node, name)) + return false; + + name = maybe_rewrite_identifier (name); symtab->change_decl_assembler_name (decl, - clone_function_name (decl, "lto_priv")); + clone_function_name_1 (name, + "lto_priv")); if (node->lto_file_data) lto_record_renamed_decl (node->lto_file_data, name, IDENTIFIER_POINTER @@ -847,7 +922,10 @@ promote_symbol (symtab_node *node) if (DECL_VISIBILITY (node->decl) == VISIBILITY_HIDDEN && DECL_VISIBILITY_SPECIFIED (node->decl) && TREE_PUBLIC (node->decl)) - return; + { + validize_symbol_for_target (node); + return; + } gcc_checking_assert (!TREE_PUBLIC (node->decl) && !DECL_EXTERNAL (node->decl)); @@ -985,7 +1063,10 @@ lto_promote_cross_file_statics (void) /* ... or if we do not partition it. This mean that it will appear in every partition refernecing it. */ || node->get_partitioning_class () != SYMBOL_PARTITION) - continue; + { + validize_symbol_for_target (node); + continue; + } promote_symbol (node); } @@ -1000,5 +1081,8 @@ lto_promote_statics_nonwpa (void) { symtab_node *node; FOR_EACH_SYMBOL (node) - rename_statics (NULL, node); + { + rename_statics (NULL, node); + validize_symbol_for_target (node); + } }