[OpenMP] Clean up variable and function names for NVPTX backend

[gtbercea]

Summary

Clean-up variable and function names.

[jlebar]

This has been tried twice before, see D29883 and D17738. I'm as unhappy about this as anyone, and personally I don't have any preference about how we try to solve it. But I think we shouldn't check this in without hearing the objections from those past attempts.

[tra]

Justin is right. I completely forgot about this. :-/
Hal offered possible solution: https://reviews.llvm.org/D17738#661115

[gtbercea]

Hi Artem, Justin,

I see that this patch is the same as the patch Arpith wanted to post a while back i.e. D17738.

Was there a consensus regarding what the right thing to do is in this case?

I'd be interested to get the ball rolling in regard to coming up with a fix for this. I see some suggestions in past patches. Some help/clarification would be much appreciated.

Thanks!

[jlebar]

I'd be interested to get the ball rolling in regard to coming up with a fix for this. I see some suggestions in past patches. Some help/clarification would be much appreciated.

Happy to help, but I'm not sure what to offer beyond the link in Art's previous comment.

[gtbercea]

In D39005#900226, @jlebar wrote:

I'd be interested to get the ball rolling in regard to coming up with a fix for this. I see some suggestions in past patches. Some help/clarification would be much appreciated.

Happy to help, but I'm not sure what to offer beyond the link in Art's previous comment.

Thanks Justin. Perhaps if we could start by clarifying this statement "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." as well as this statement "This seems practical. Perhaps it could be part of the name mangling scheme already encoded in DataLayout?".

The first question that comes to mind is what is the link between data layout and name mangling conventions?

[jlebar]

The first question that comes to mind is what is the link between data layout and name mangling conventions?

I pulled up http://llvm.org/doxygen/classllvm_1_1DataLayout.html and searched for "mangling" -- presumably this is what they were referring to. We also don't need to speculate, rnk still works on LLVM. :)

[hfinkel]

In D39005#900973, @jlebar wrote:

The first question that comes to mind is what is the link between data layout and name mangling conventions?

I pulled up http://llvm.org/doxygen/classllvm_1_1DataLayout.html and searched for "mangling" -- presumably this is what they were referring to. We also don't need to speculate, rnk still works on LLVM. :)

DataLayout generally holds information that the target-independent optimizer needs in order to simplify the IR into our canonical form. This is as opposed to TargetTransformInfo, which provides data necessary to optimize the IR in target-aware ways (e.g., do things that are orthogonal to canonicalization such as inlining and vectorization). It is also as opposed to external utility functions that might be used by the frontend (e.g., llvm::sys::getHostCPUName()). If I recall correctly, this is information that would be used by the frontend when generating the IR, and the function results are controlled by the triple. As a result, I think that a general utility function somewhere would be fine.

[Hahnfeld]

Fixed differently in rL319657.

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);
+  /// 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()));
-    }
-  }
+  for (GlobalVariable &GV : M.globals())
+    if (GV.hasLocalLinkage())
+      generateCleanName(GV);
+
+  // Clean function symbols.
+  for (auto &FN : M.functions())
+    if (FN.hasLocalLinkage())
+      generateCleanName(FN);
 
   return true;
 }
 
+void NVPTXAssignValidGlobalNames::generateCleanName(Value &V) {
+  std::string ValidName;
+  do {
+    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.
+  } while (!V.getName().equals(ValidName));
+}
+
 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 {