[Mlir-commits] [mlir] [MLIR][LLVM] DI Recursive Type fix for recursion via scope of composites (PR #85850)

Billy Zhu llvmlistbot at llvm.org
Tue Mar 19 13:27:10 PDT 2024


https://github.com/zyx-billy updated https://github.com/llvm/llvm-project/pull/85850

>From b6277c24a60000d14354448892ed2f949e8e2629 Mon Sep 17 00:00:00 2001
From: Billy Zhu <billyzhu at modular.com>
Date: Tue, 19 Mar 2024 11:58:32 -0700
Subject: [PATCH 1/3] create temporary instead

---
 mlir/lib/Target/LLVMIR/DebugTranslation.cpp | 50 +++++++++++----------
 mlir/lib/Target/LLVMIR/DebugTranslation.h   | 24 ++++------
 mlir/test/Target/LLVMIR/llvmir-debug.mlir   | 21 +++++++++
 3 files changed, 55 insertions(+), 40 deletions(-)

diff --git a/mlir/lib/Target/LLVMIR/DebugTranslation.cpp b/mlir/lib/Target/LLVMIR/DebugTranslation.cpp
index 1248de3db07048..126a671a58e808 100644
--- a/mlir/lib/Target/LLVMIR/DebugTranslation.cpp
+++ b/mlir/lib/Target/LLVMIR/DebugTranslation.cpp
@@ -116,8 +116,20 @@ static DINodeT *getDistinctOrUnique(bool isDistinct, Ts &&...args) {
   return DINodeT::get(std::forward<Ts>(args)...);
 }
 
+llvm::TempDICompositeType
+DebugTranslation::translateTemporaryImpl(DICompositeTypeAttr attr) {
+  return llvm::DICompositeType::getTemporary(
+      llvmCtx, attr.getTag(), getMDStringOrNull(attr.getName()), nullptr,
+      attr.getLine(), nullptr, nullptr, attr.getSizeInBits(),
+      attr.getAlignInBits(),
+      /*OffsetInBits=*/0,
+      /*Flags=*/static_cast<llvm::DINode::DIFlags>(attr.getFlags()),
+      /*Elements=*/nullptr, /*RuntimeLang=*/0,
+      /*VTableHolder=*/nullptr);
+}
+
 llvm::DICompositeType *
-DebugTranslation::translateImplGetPlaceholder(DICompositeTypeAttr attr) {
+DebugTranslation::translateImpl(DICompositeTypeAttr attr) {
   // TODO: Use distinct attributes to model this, once they have landed.
   // Depending on the tag, composite types must be distinct.
   bool isDistinct = false;
@@ -129,8 +141,10 @@ DebugTranslation::translateImplGetPlaceholder(DICompositeTypeAttr attr) {
     isDistinct = true;
   }
 
-  llvm::TempMDTuple placeholderElements =
-      llvm::MDNode::getTemporary(llvmCtx, std::nullopt);
+  SmallVector<llvm::Metadata *> elements;
+  for (DINodeAttr member : attr.getElements())
+    elements.push_back(translate(member));
+
   return getDistinctOrUnique<llvm::DICompositeType>(
       isDistinct, llvmCtx, attr.getTag(), getMDStringOrNull(attr.getName()),
       translate(attr.getFile()), attr.getLine(), translate(attr.getScope()),
@@ -138,23 +152,8 @@ DebugTranslation::translateImplGetPlaceholder(DICompositeTypeAttr attr) {
       attr.getAlignInBits(),
       /*OffsetInBits=*/0,
       /*Flags=*/static_cast<llvm::DINode::DIFlags>(attr.getFlags()),
-      /*Elements=*/placeholderElements.get(), /*RuntimeLang=*/0,
-      /*VTableHolder=*/nullptr);
-}
-
-void DebugTranslation::translateImplFillPlaceholder(
-    DICompositeTypeAttr attr, llvm::DICompositeType *placeholder) {
-  SmallVector<llvm::Metadata *> elements;
-  for (DINodeAttr member : attr.getElements())
-    elements.push_back(translate(member));
-  placeholder->replaceElements(llvm::MDNode::get(llvmCtx, elements));
-}
-
-llvm::DICompositeType *
-DebugTranslation::translateImpl(DICompositeTypeAttr attr) {
-  llvm::DICompositeType *placeholder = translateImplGetPlaceholder(attr);
-  translateImplFillPlaceholder(attr, placeholder);
-  return placeholder;
+      llvm::MDNode::get(llvmCtx, elements),
+      /*RuntimeLang=*/0, /*VTableHolder=*/nullptr);
 }
 
 llvm::DIDerivedType *DebugTranslation::translateImpl(DIDerivedTypeAttr attr) {
@@ -234,10 +233,13 @@ DebugTranslation::translateRecursive(DIRecursiveTypeAttrInterface attr) {
   llvm::DIType *result =
       TypeSwitch<DIRecursiveTypeAttrInterface, llvm::DIType *>(attr)
           .Case<DICompositeTypeAttr>([&](auto attr) {
-            auto *placeholder = translateImplGetPlaceholder(attr);
-            setRecursivePlaceholder(placeholder);
-            translateImplFillPlaceholder(attr, placeholder);
-            return placeholder;
+            auto temporary = translateTemporaryImpl(attr);
+            setRecursivePlaceholder(temporary.get());
+            // Must call `translateImpl` directly instead of `translate` to
+            // avoid handling the recursive interface again.
+            auto *concrete = translateImpl(attr);
+            temporary->replaceAllUsesWith(concrete);
+            return concrete;
           });
 
   assert(recursiveTypeMap.back().first == recursiveId &&
diff --git a/mlir/lib/Target/LLVMIR/DebugTranslation.h b/mlir/lib/Target/LLVMIR/DebugTranslation.h
index d2171fed8c594f..c7a5228cbf5e92 100644
--- a/mlir/lib/Target/LLVMIR/DebugTranslation.h
+++ b/mlir/lib/Target/LLVMIR/DebugTranslation.h
@@ -88,23 +88,16 @@ class DebugTranslation {
   llvm::DISubroutineType *translateImpl(DISubroutineTypeAttr attr);
   llvm::DIType *translateImpl(DITypeAttr attr);
 
-  /// Attributes that support self recursion need to implement two methods and
-  /// hook into the `translateImpl` overload for `DIRecursiveTypeAttr`.
-  /// - `<llvm type> translateImplGetPlaceholder(<mlir type>)`:
-  ///   Translate the DI attr without translating any potentially recursive
-  ///   nested DI attrs.
-  /// - `void translateImplFillPlaceholder(<mlir type>, <llvm type>)`:
-  ///   Given the placeholder returned by `translateImplGetPlaceholder`, fill
-  ///   any holes by recursively translating nested DI attrs. This method must
-  ///   mutate the placeholder that is passed in, instead of creating a new one.
+  /// Attributes that support self recursion need to implement an additional
+  /// method to hook into `translateRecursive`.
+  /// - `<temp llvm type> translateTemporaryImpl(<mlir type>)`:
+  ///   Create a temporary translation of the DI attr without recursively
+  ///   translating any nested DI attrs.
   llvm::DIType *translateRecursive(DIRecursiveTypeAttrInterface attr);
 
-  /// Get a placeholder DICompositeType without recursing into the elements.
-  llvm::DICompositeType *translateImplGetPlaceholder(DICompositeTypeAttr attr);
-  /// Completes the DICompositeType `placeholder` by recursively translating the
-  /// elements.
-  void translateImplFillPlaceholder(DICompositeTypeAttr attr,
-                                    llvm::DICompositeType *placeholder);
+  /// Translate the given attribute to a temporary llvm debug metadata of the
+  /// corresponding type.
+  llvm::TempDICompositeType translateTemporaryImpl(DICompositeTypeAttr attr);
 
   /// Constructs a string metadata node from the string attribute. Returns
   /// nullptr if `stringAttr` is null or contains and empty string.
@@ -121,7 +114,6 @@ class DebugTranslation {
   DenseMap<Attribute, llvm::DINode *> attrToNode;
 
   /// A mapping between recursive ID and the translated DIType.
-  /// DIType.
   llvm::MapVector<DistinctAttr, llvm::DIType *> recursiveTypeMap;
 
   /// A mapping between a distinct ID and the translated LLVM metadata node.
diff --git a/mlir/test/Target/LLVMIR/llvmir-debug.mlir b/mlir/test/Target/LLVMIR/llvmir-debug.mlir
index 5d70bff52bb2b9..33faa95d7d1967 100644
--- a/mlir/test/Target/LLVMIR/llvmir-debug.mlir
+++ b/mlir/test/Target/LLVMIR/llvmir-debug.mlir
@@ -402,3 +402,24 @@ llvm.func @func_debug_directives() {
 llvm.func @class_method() {
   llvm.return loc(#loc3)
 } loc(#loc3)
+
+// -----
+
+// Test recursive composite type with recursion via its scope is translated.
+
+#di_composite_type_self = #llvm.di_composite_type<tag = DW_TAG_null, recId = distinct[0]<>>
+#di_file = #llvm.di_file<"test.mlir" in "/">
+#di_subroutine_type = #llvm.di_subroutine_type<types = #di_composite_type_self>
+#di_subprogram = #llvm.di_subprogram<scope = #di_file, name = "my_func", file = #di_file, line = 182, scopeLine = 182, subprogramFlags = Optimized, type = #di_subroutine_type>
+// The composite type whose scope leads to a self-recursive cycle.
+#di_composite_type = #llvm.di_composite_type<tag = DW_TAG_class_type, recId = distinct[0]<>, name = "id", file = #di_file, line = 204, scope = #di_subprogram, flags = "Public|TypePassByReference|NonTrivial", sizeInBits = 128>
+#di_global_variable = #llvm.di_global_variable<scope = #di_file, name = "my_global", file = #di_file, line = 4641, type = #di_composite_type, isDefined = true>
+#di_global_variable_expression = #llvm.di_global_variable_expression<var = #di_global_variable, expr = <>>
+
+llvm.mlir.global @global_variable() {dbg_expr = #di_global_variable_expression} : !llvm.struct<(struct<(i64)>, i32)>
+
+// CHECK: distinct !DIGlobalVariable(name: "my_global", {{.*}}, type: ![[COMP:[0-9]+]],
+// CHECK: ![[COMP]] = distinct !DICompositeType(tag: DW_TAG_class_type, name: "id", scope: ![[SCOPE:[0-9]+]],
+// CHECK: ![[SCOPE]] = !DISubprogram(name: "my_func", {{.*}}, type: ![[SUBROUTINE:[0-9]+]],
+// CHECK: ![[SUBROUTINE]] = !DISubroutineType(types: ![[SR_TYPES:[0-9]+]])
+// CHECK: ![[SR_TYPES]] = !{![[COMP]]}

>From f3862e44db6243395f70563311d2fe210bb07548 Mon Sep 17 00:00:00 2001
From: Billy Zhu <billyzhu at modular.com>
Date: Tue, 19 Mar 2024 13:20:41 -0700
Subject: [PATCH 2/3] Apply suggestions from code review

Co-authored-by: Tobias Gysi <tobias.gysi at nextsilicon.com>
---
 mlir/test/Target/LLVMIR/llvmir-debug.mlir | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/mlir/test/Target/LLVMIR/llvmir-debug.mlir b/mlir/test/Target/LLVMIR/llvmir-debug.mlir
index 33faa95d7d1967..d6ad0b3d6e57fc 100644
--- a/mlir/test/Target/LLVMIR/llvmir-debug.mlir
+++ b/mlir/test/Target/LLVMIR/llvmir-debug.mlir
@@ -405,13 +405,12 @@ llvm.func @class_method() {
 
 // -----
 
-// Test recursive composite type with recursion via its scope is translated.
+// Ensures composite types with a recursive scope work.
 
 #di_composite_type_self = #llvm.di_composite_type<tag = DW_TAG_null, recId = distinct[0]<>>
 #di_file = #llvm.di_file<"test.mlir" in "/">
 #di_subroutine_type = #llvm.di_subroutine_type<types = #di_composite_type_self>
 #di_subprogram = #llvm.di_subprogram<scope = #di_file, name = "my_func", file = #di_file, line = 182, scopeLine = 182, subprogramFlags = Optimized, type = #di_subroutine_type>
-// The composite type whose scope leads to a self-recursive cycle.
 #di_composite_type = #llvm.di_composite_type<tag = DW_TAG_class_type, recId = distinct[0]<>, name = "id", file = #di_file, line = 204, scope = #di_subprogram, flags = "Public|TypePassByReference|NonTrivial", sizeInBits = 128>
 #di_global_variable = #llvm.di_global_variable<scope = #di_file, name = "my_global", file = #di_file, line = 4641, type = #di_composite_type, isDefined = true>
 #di_global_variable_expression = #llvm.di_global_variable_expression<var = #di_global_variable, expr = <>>

>From ef9017e78c19d53d8f52da07892277e79583b0d9 Mon Sep 17 00:00:00 2001
From: Billy Zhu <billyzhu at modular.com>
Date: Tue, 19 Mar 2024 13:26:48 -0700
Subject: [PATCH 3/3] strip all optional params from test case

---
 mlir/test/Target/LLVMIR/llvmir-debug.mlir | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/mlir/test/Target/LLVMIR/llvmir-debug.mlir b/mlir/test/Target/LLVMIR/llvmir-debug.mlir
index d6ad0b3d6e57fc..c042628334d4c5 100644
--- a/mlir/test/Target/LLVMIR/llvmir-debug.mlir
+++ b/mlir/test/Target/LLVMIR/llvmir-debug.mlir
@@ -410,15 +410,15 @@ llvm.func @class_method() {
 #di_composite_type_self = #llvm.di_composite_type<tag = DW_TAG_null, recId = distinct[0]<>>
 #di_file = #llvm.di_file<"test.mlir" in "/">
 #di_subroutine_type = #llvm.di_subroutine_type<types = #di_composite_type_self>
-#di_subprogram = #llvm.di_subprogram<scope = #di_file, name = "my_func", file = #di_file, line = 182, scopeLine = 182, subprogramFlags = Optimized, type = #di_subroutine_type>
-#di_composite_type = #llvm.di_composite_type<tag = DW_TAG_class_type, recId = distinct[0]<>, name = "id", file = #di_file, line = 204, scope = #di_subprogram, flags = "Public|TypePassByReference|NonTrivial", sizeInBits = 128>
-#di_global_variable = #llvm.di_global_variable<scope = #di_file, name = "my_global", file = #di_file, line = 4641, type = #di_composite_type, isDefined = true>
-#di_global_variable_expression = #llvm.di_global_variable_expression<var = #di_global_variable, expr = <>>
+#di_subprogram = #llvm.di_subprogram<scope = #di_file, file = #di_file, subprogramFlags = Optimized, type = #di_subroutine_type>
+#di_composite_type = #llvm.di_composite_type<tag = DW_TAG_class_type, recId = distinct[0]<>, scope = #di_subprogram>
+#di_global_variable = #llvm.di_global_variable<file = #di_file, line = 1, type = #di_composite_type>
+#di_global_variable_expression = #llvm.di_global_variable_expression<var = #di_global_variable>
 
-llvm.mlir.global @global_variable() {dbg_expr = #di_global_variable_expression} : !llvm.struct<(struct<(i64)>, i32)>
+llvm.mlir.global @global_variable() {dbg_expr = #di_global_variable_expression} : !llvm.struct<()>
 
-// CHECK: distinct !DIGlobalVariable(name: "my_global", {{.*}}, type: ![[COMP:[0-9]+]],
-// CHECK: ![[COMP]] = distinct !DICompositeType(tag: DW_TAG_class_type, name: "id", scope: ![[SCOPE:[0-9]+]],
-// CHECK: ![[SCOPE]] = !DISubprogram(name: "my_func", {{.*}}, type: ![[SUBROUTINE:[0-9]+]],
+// CHECK: distinct !DIGlobalVariable({{.*}}type: ![[COMP:[0-9]+]],
+// CHECK: ![[COMP]] = distinct !DICompositeType({{.*}}scope: ![[SCOPE:[0-9]+]],
+// CHECK: ![[SCOPE]] = !DISubprogram({{.*}}type: ![[SUBROUTINE:[0-9]+]],
 // CHECK: ![[SUBROUTINE]] = !DISubroutineType(types: ![[SR_TYPES:[0-9]+]])
 // CHECK: ![[SR_TYPES]] = !{![[COMP]]}



More information about the Mlir-commits mailing list