diff mbox

Another ptx offloading patch

Message ID 546DDFB4.9050302@codesourcery.com
State New
Headers show

Commit Message

Bernd Schmidt Nov. 20, 2014, 12:33 p.m. UTC
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?


Bernd

Comments

Jeff Law Jan. 15, 2015, 6:35 p.m. UTC | #1
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
Richard Biener Jan. 16, 2015, 10:45 a.m. UTC | #2
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
>
Jakub Jelinek Feb. 4, 2015, 10:38 a.m. UTC | #3
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
Richard Biener Feb. 10, 2015, 10:34 a.m. UTC | #4
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
Bernd Schmidt Feb. 10, 2015, 12:06 p.m. UTC | #5
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
Jakub Jelinek Feb. 10, 2015, 12:10 p.m. UTC | #6
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
diff mbox

Patch

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);
+    }
 }