[NVPTX] Fix function identifiers that are invalid in PTX and a bug fix for the case of name collisions.

[arpith-jacob]

Summary

The characters '.' and '@' are not allowed by PTX in identifiers. A previous patch cleans up such identifiers for global variables. This patch cleans up identifiers of local functions as well.

An unrelated patch changed the name collision avoidance code in LLVM by adding a suffix of the pattern ["." number]. This broke NVPTXAssignValidGlobalNames.cpp as the dot is introduced in the case of collisions. I fix this by cleaning up the new name and repeating as required.

[jlebar]

lib/Target/NVPTX/NVPTXAssignValidGlobalNames.cpp

40

Nit: "name, ensuring", since the gerund phrase is an appositive.

74

cleanUpName returns a string -- isn't this a dangling pointer?

[• rafael]

Wouldn't it be better to error and ask the llvm producer to not create
global values with '@' and '.' when targeting nvptx?

Cheers,
Rafael

[jlebar]

In D17738#364717, @rafael wrote:

Wouldn't it be better to error and ask the llvm producer to not create
global values with '@' and '.' when targeting nvptx?

rL205212 suggests we already do generic sanitizing for all backends. If that's true, why is it inappropriate to do target-specific sanitization in addition? That seems cleaner than playing whack-a-mole in the frontend so it doesn't emit names that aren't valid, when compiling for this target.

[arpith-jacob]

Don't dangle pointers :(

[arpith-jacob]

In D17738#364717, @rafael wrote:

Wouldn't it be better to error and ask the llvm producer to not create
global values with '@' and '.' when targeting nvptx?

Unfortunately the llvm producer changed, which broke code for this target without anyone noticing :(

lib/Target/NVPTX/NVPTXAssignValidGlobalNames.cpp

74

Ouch!

[jlebar]

lgtm, but I'd like Rafael to indicate that his concern is resolved.

[arpith-jacob]

Friendly ping. Rafael, do you have further concerns? Thanks.

[• rafael]

Wouldn't it be better to error and ask the llvm producer to not create
global values with '@' and '.' when targeting nvptx?

http://reviews.llvm.org/rL205212 suggests we already do generic sanitizing for all backends. If that's true, why is it inappropriate to do target-specific sanitization in addition? That seems cleaner than playing whack-a-mole in the frontend so it doesn't emit names that aren't valid, when compiling for this target.

I disagree. LLVM has a bad history of playing whack-a-mole with what
names are supported by various formats and assemblers and renaming
stuff in the back. This creates problems with collisions (like you
found) and for any tools that needs to find the final name, like LTO.

The real stable solution I think is for the IR name to be the final
name. No further mangling involved. It will take a lot of work to get
there, but we can avoid making the situation worse.

Cheers,
Rafael

[arpith-jacob]

In D17738#367459, @rafael wrote:

The real stable solution I think is for the IR name to be the final
name. No further mangling involved.

Correct me if I am wrong, but I think you are saying that the llvm IR name should only contain chars that are acceptable for *all* targets.

In this example, for valid ptx code, it's important that the period never appears in the IR name.

However, according to this patch:

For globals it is important to use "foo.1" to help C++ name demangling.

So the two consumers have opposing constraints and the suggested approach cannot work?

[rnk]

In D17738#367459, @rafael wrote:

The real stable solution I think is for the IR name to be the final
name. No further mangling involved. It will take a lot of work to get
there, but we can avoid making the situation worse.

The problem there is that we would have to make the IR symbol renamer not append '.N' when renaming symbols with name collisions. Until then, we need something like this.

I thought we used to have machinery in our assembler for mangling away invalid characters. Did you remove that stuff?

[hfinkel]

In D17738#416060, @rnk wrote:

In D17738#367459, @rafael wrote:

The real stable solution I think is for the IR name to be the final
name. No further mangling involved. It will take a lot of work to get
there, but we can avoid making the situation worse.

The problem there is that we would have to make the IR symbol renamer not append '.N' when renaming symbols with name collisions. Until then, we need something like this.

I thought we used to have machinery in our assembler for mangling away invalid characters. Did you remove that stuff?

How would we like to move forward here?

What @rafael said, that we should not re-mangle IR names, sounds appealing. This holds obvious tooling advantages. It is also true that we need some way to generate unique symbol names that does not interfere with existing mangling schemes. This means that we need to have some character that is not in [A-Za-z0-9_], and is not @ (because that's used by ELF symbol versioning). ?$@' are used by MSVC mangling. PTX allows only [A-Za-z0-9_$] (and also %, but only as the first character, so that doesn't help). Unfortunately, this does not seem to leave anything we can use (we can use '$' for PTX, but not for MSVC).

One option is that we add a function to LLVM get an available separator character, which can default to '.', but we set to '$' for nvptx, and use that for generating new names at the IR level. Thoughts?

[rnk]

In D17738#661115, @hfinkel wrote:

One option is that we add a function to LLVM get an available separator character, which can default to '.', but we set to '$' for nvptx, and use that for generating new names at the IR level. Thoughts?

This seems practical. Perhaps it could be part of the name mangling scheme already encoded in DataLayout?

lib/Target/NVPTX/NVPTXAssignValidGlobalNames.cpp

 public:
   static char ID;
   NVPTXAssignValidGlobalNames() : ModulePass(ID) {}
 
   bool runOnModule(Module &M) override;
 
   /// \brief Clean up the name to remove symbols invalid in PTX.
   std::string cleanUpName(StringRef Name);
+  /// \brief Set a clean name, ensuring collisions are avoided.
+  void generateCleanName(Value &V);
 };
 }
 
 char NVPTXAssignValidGlobalNames::ID = 0;
 
 namespace llvm {
 void initializeNVPTXAssignValidGlobalNamesPass(PassRegistry &);
 }
 
 INITIALIZE_PASS(NVPTXAssignValidGlobalNames, "nvptx-assign-valid-global-names",
                 "Assign valid PTX names to globals", false, false)
 
 bool NVPTXAssignValidGlobalNames::runOnModule(Module &M) {
   for (GlobalVariable &GV : M.globals()) {
     // We are only allowed to rename local symbols.
     if (GV.hasLocalLinkage()) {
-      // setName doesn't do extra work if the name does not change.
-      // Note: this does not create collisions - if setName is asked to set the
-      // name to something that already exists, it adds a proper postfix to
-      // avoid collisions.
-      GV.setName(cleanUpName(GV.getName()));
+      generateCleanName(GV);
+    }
+  }
+
+  // Clean function symbols.
+  for (auto &FN : M.functions()) {
+    if (FN.hasLocalLinkage()) {
+      generateCleanName(FN);
     }
   }
 
   return true;
 }
 
+void NVPTXAssignValidGlobalNames::generateCleanName(Value &V) {
+  while (1) {
+    std::string ValidName = cleanUpName(V.getName());
+    // setName doesn't do extra work if the name does not change.
+    // Collisions are avoided by adding a suffix (which may yet be unclean in
+    // PTX).
+    V.setName(ValidName);
+    // If there are no collisions return, otherwise clean up the new name.
+    if (V.getName().equals(ValidName))
+      return;
+  }
+}
+
 std::string NVPTXAssignValidGlobalNames::cleanUpName(StringRef Name) {
   std::string ValidName;
   raw_string_ostream ValidNameStream(ValidName);
   for (unsigned I = 0, E = Name.size(); I != E; ++I) {
     char C = Name[I];
     if (C == '.' || C == '@') {
       ValidNameStream << "_$_";
     } else {

test/CodeGen/NVPTX/symbol-naming.ll

 ; RUN: llc < %s -march=nvptx -mcpu=sm_20 | FileCheck %s --check-prefix=PTX32
 ; RUN: llc < %s -march=nvptx64 -mcpu=sm_20 | FileCheck %s --check-prefix=PTX64
 
 ; Verify that the NVPTX target removes invalid symbol names prior to emitting
 ; PTX.
+; A symbol is invalid in PTX if it contains a '.' or a '@'.
 
 ; PTX32-NOT: .str
 ; PTX64-NOT: .str
 
-; PTX32-DAG: _$_str.1
+; PTX32-DAG: _$_str_$_1
 ; PTX32-DAG: _$_str
 
-; PTX64-DAG: _$_str.1
+; PTX64-DAG: _$_str_$_1
 ; PTX64-DAG: _$_str
 
 target datalayout = "e-i64:64-v16:16-v32:32-n16:32:64"
 target triple = "nvptx64-unknown-unknown"
 
 
 @.str = private unnamed_addr constant [13 x i8] c"%d %f %c %d\0A\00", align 1
 @_$_str = private unnamed_addr constant [13 x i8] c"%d %f %c %d\0A\00", align 1
 
 
 ; Function Attrs: nounwind
 define void @foo(i32 %a, float %b, i8 signext %c, i32 %e) {
 entry:
   %call = call i32 (i8*, ...) @printf(i8* getelementptr inbounds ([13 x i8], [13 x i8]* @.str, i32 0, i32 0))
   ret void
 }
 
+; PTX32-NOT: omp_outlined.1
+; PTX64-NOT: omp_outlined.1
+; PTX32-DAG: omp_outlined_$_1_$_2_$_3
+; PTX64-DAG: omp_outlined_$_1_$_2_$_3
+define internal void @omp_outlined.1() {
+  ret void
+}
+
+declare void @omp_outlined_$_1()
+declare void @omp_outlined_$_1_$_2()
+
 declare i32 @printf(i8*, ...)