[NVPTX] Fixing debug symbols for ptx target emitting #101891

vyom1611 · 2024-08-04T14:54:11Z

Based on issue #101819

github-actions · 2024-08-04T14:54:29Z

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be
notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write
permissions for the repository. In which case you can instead tag reviewers by
name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review
by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate
is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

llvmbot · 2024-08-04T14:54:59Z

@llvm/pr-subscribers-backend-nvptx

Author: Vyom Sharma (vyom1611)

Changes

Based on issue #101819

Full diff: https://github.com/llvm/llvm-project/pull/101891.diff

1 Files Affected:

(modified) llvm/lib/Target/NVPTX/NVPTXAssignValidGlobalNames.cpp (+47)

diff --git a/llvm/lib/Target/NVPTX/NVPTXAssignValidGlobalNames.cpp b/llvm/lib/Target/NVPTX/NVPTXAssignValidGlobalNames.cpp
index 9f5a217795183..b687e6ceba504 100644
--- a/llvm/lib/Target/NVPTX/NVPTXAssignValidGlobalNames.cpp
+++ b/llvm/lib/Target/NVPTX/NVPTXAssignValidGlobalNames.cpp
@@ -18,6 +18,7 @@
 
 #include "NVPTX.h"
 #include "llvm/ADT/StringExtras.h"
+#include "llvm/IR/DebugInfoMetadata.h"
 #include "llvm/IR/Function.h"
 #include "llvm/IR/GlobalVariable.h"
 #include "llvm/IR/LegacyPassManager.h"
@@ -38,6 +39,9 @@ class NVPTXAssignValidGlobalNames : public ModulePass {
 
   /// Clean up the name to remove symbols invalid in PTX.
   std::string cleanUpName(StringRef Name);
+  
+  /// Clean up the debug symbols.
+  void cleanUpDebugSymbols(Module &M);
 };
 }
 
@@ -67,6 +71,9 @@ bool NVPTXAssignValidGlobalNames::runOnModule(Module &M) {
     if (F.hasLocalLinkage())
       F.setName(cleanUpName(F.getName()));
 
+  // Clean up the debug symbols.
+  cleanUpDebugSymbols(M);
+
   return true;
 }
 
@@ -86,6 +93,46 @@ std::string NVPTXAssignValidGlobalNames::cleanUpName(StringRef Name) {
   return ValidNameStream.str();
 }
 
+void NVPTXAssignValidGlobalNames::cleanUpDebugSymbols(Module &M) {
+  LLVMContext &Ctx = M.getContext();
+  
+  for (Function &F : M.functions()) {
+    if (DISubprogram *SP = F.getSubprogram()) {
+      auto CleanedName = cleanUpName(SP->getLinkageName());
+      if (!CleanedName.empty()) {
+        SP->replaceLinkageName(MDString::get(Ctx, CleanedName));
+      }
+    }
+  }
+
+  for (GlobalVariable &GV : M.globals()) {
+    SmallVector<DIGlobalVariableExpression *, 1> GVs;
+    GV.getDebugInfo(GVs);
+    for (auto *GVE : GVs) {
+      DIGlobalVariable *GVMD = GVE->getVariable();
+      auto CleanedName = cleanUpName(GVMD->getLinkageName());
+      if (!CleanedName.empty()) {
+        DIGlobalVariable *NewGVMD = DIGlobalVariable::get(
+          Ctx,
+          GVMD->getScope(),
+          GVMD->getName(),
+          CleanedName, // Use the cleaned name as StringRef
+          GVMD->getFile(),
+          GVMD->getLine(),
+          GVMD->getType(),
+          GVMD->isLocalToUnit(),
+          GVMD->isDefinition(),
+          GVMD->getStaticDataMemberDeclaration(),
+          GVMD->getTemplateParams(),
+          GVMD->getAlignInBits(),
+          GVMD->getAnnotations()
+        );
+        GVMD->replaceAllUsesWith(NewGVMD);
+      }
+    }
+  }
+}
+
 ModulePass *llvm::createNVPTXAssignValidGlobalNamesPass() {
   return new NVPTXAssignValidGlobalNames();
 }

enferex · 2024-08-05T11:37:13Z

Please add a lit test that exercises this change.

vyom1611 · 2024-08-10T08:29:30Z

@enferex clang-format has been done on the changed file

enferex · 2024-08-10T10:57:09Z

Thanks @vyom1611, can you please provide a lit test.

vyom1611 · 2024-08-10T11:25:26Z

Hey, sorry I thought you said lint test. BTW, when you say add a lit test, do I need to create another test in the test directory for this change? Or just run the lit test and show if all the tests pass. Sorry I have never contributed to llvm and I do not know what the usual workflow is.

enferex · 2024-08-10T11:50:16Z

Hey, sorry I thought you said lint test. BTW, when you say add a lit test, do I need to create another test in the test directory for this change? Or just run the lit test and show if all the tests pass. Sorry I have never contributed to llvm and I do not know what the usual workflow is.

No problem. Yes, I'd like to see a lit test. This will help myself and others better understand the behavior of your change, as well as prevent regressions if this change is approved. The LLVM docs (link) contain a guide for submitting patches.

jhuber6 · 2024-08-14T19:09:12Z

Hey, sorry I thought you said lint test. BTW, when you say add a lit test, do I need to create another test in the test directory for this change? Or just run the lit test and show if all the tests pass. Sorry I have never contributed to llvm and I do not know what the usual workflow is.

You want to create some minimal LLVM-IR that reproduces the issue, checking the output of the failing test case is a good start https://godbolt.org/z/aYh8WGrj3. Look at llvm/test/CodeGen/NVPTX for similar tests. These will run llc on the IR to turn it into PTX. The test should verify that the debug symbol's name matches the global string.

Artem-B · 2024-08-14T19:25:20Z

Here's a small reproducer: https://godbolt.org/z/3jWrKWeEM

Artem-B

LGTM in principle, Should be good to go, once we have the test.

vyom1611 · 2024-08-18T06:33:55Z

Unit test passes