[mlir][LLVM] Improve function debug info import

[gysit]

This commit improves the import of function debug info by creating a FileLineColLoc instead of just a NameLoc if possible.

[llvmbot]

@llvm/pr-subscribers-mlir

@llvm/pr-subscribers-mlir-llvm

Author: Tobias Gysi (gysit)

Changes

This commit improves the import of function debug info by creating a FileLineColLoc instead of just a NameLoc if possible.

---

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

2 Files Affected:

• (modified) mlir/lib/Target/LLVMIR/DebugImporter.cpp (+11-5)
• (modified) mlir/test/Target/LLVMIR/Import/debug-info.ll (+7-4)

diff --git a/mlir/lib/Target/LLVMIR/DebugImporter.cpp b/mlir/lib/Target/LLVMIR/DebugImporter.cpp
index 695dbf75a448125..0f4c4f454860bd9 100644
--- a/mlir/lib/Target/LLVMIR/DebugImporter.cpp
+++ b/mlir/lib/Target/LLVMIR/DebugImporter.cpp
@@ -24,13 +24,19 @@ using namespace mlir::LLVM;
 using namespace mlir::LLVM::detail;
 
 Location DebugImporter::translateFuncLocation(llvm::Function *func) {
-  if (!func->getSubprogram())
+  llvm::DISubprogram *subprogram = func->getSubprogram();
+  if (!subprogram)
     return UnknownLoc::get(context);
 
-  // Add a fused location to link the subprogram information.
-  StringAttr name = StringAttr::get(context, func->getSubprogram()->getName());
-  return FusedLocWith<DISubprogramAttr>::get(
-      {NameLoc::get(name)}, translate(func->getSubprogram()), context);
+  SmallVector<Location, 2> funcLocs = {
+      NameLoc::get(StringAttr::get(context, subprogram->getName()))};
+  if (llvm::DIScope *scope = subprogram->getScope())
+    funcLocs.push_back(
+        FileLineColLoc::get(StringAttr::get(context, scope->getFilename()),
+                            subprogram->getLine(), /*column=*/0));
+
+  return FusedLocWith<DISubprogramAttr>::get(funcLocs, translate(subprogram),
+                                             context);
 }
 
 //===----------------------------------------------------------------------===//
diff --git a/mlir/test/Target/LLVMIR/Import/debug-info.ll b/mlir/test/Target/LLVMIR/Import/debug-info.ll
index da8f7c3e8308721..e9f53b1546596db 100644
--- a/mlir/test/Target/LLVMIR/Import/debug-info.ll
+++ b/mlir/test/Target/LLVMIR/Import/debug-info.ll
@@ -222,15 +222,18 @@ define void @subprogram() !dbg !3 {
 define void @func_loc() !dbg !3 {
   ret void
 }
-; CHECK: #[[SP:.+]] =  #llvm.di_subprogram<compileUnit = #{{.*}}, scope = #{{.*}}, name = "func_loc", file = #{{.*}}, subprogramFlags = Definition>
-; CHECK: loc(fused<#[[SP]]>[
+; CHECK-DAG: #[[NAME_LOC:.+]] = loc("func_loc")
+; CHECK-DAG: #[[FILE_LOC:.+]] = loc("debug-info.ll":42:0)
+; CHECK-DAG: #[[SP:.+]] =  #llvm.di_subprogram<compileUnit = #{{.*}}, scope = #{{.*}}, name = "func_loc", file = #{{.*}}, line = 42, subprogramFlags = Definition>
+
+; CHECK: loc(fused<#[[SP]]>[#[[NAME_LOC]],  #[[FILE_LOC]]]
 
 !llvm.dbg.cu = !{!1}
 !llvm.module.flags = !{!0}
 !0 = !{i32 2, !"Debug Info Version", i32 3}
 !1 = distinct !DICompileUnit(language: DW_LANG_C, file: !2)
 !2 = !DIFile(filename: "debug-info.ll", directory: "/")
-!3 = distinct !DISubprogram(name: "func_loc", scope: !2, file: !2, spFlags: DISPFlagDefinition, unit: !1)
+!3 = distinct !DISubprogram(name: "func_loc", scope: !2, file: !2, spFlags: DISPFlagDefinition, unit: !1, line: 42)
 
 ; // -----
 
@@ -538,7 +541,7 @@ define void @noname_subprogram(ptr %arg) !dbg !8 {
 ; // -----
 
 ; CHECK:      #[[MODULE:.+]] = #llvm.di_module<
-; CHECK-SAME: file = #{{.*}}, scope = #{{.*}}, name = "module", 
+; CHECK-SAME: file = #{{.*}}, scope = #{{.*}}, name = "module",
 ; CHECK-SAME: configMacros = "bar", includePath = "/",
 ; CHECK-SAME: apinotes = "/", line = 42, isDecl = true
 ; CHECK-SAME: >

[Dinistro]

Is that double space intended?

[Dinistro]

Is there a guarantee that the scope is always a DIFile or can it also be a DINamespace or a DIModule? Maybe relying on the file field might be safer.

[gysit]

nice I did not realize the subprogram has a filename getter itself. The getters all return an empty string if the filename is not available.

[Dinistro]

LGTM!