[MLIR][LLVM] DI Recursive Type fix for recursion via scope of composites

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,32 +141,19 @@ 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()),
       translate(attr.getBaseType()), attr.getSizeInBits(),
       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 &&

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.

mlir/test/Target/LLVMIR/llvmir-debug.mlir

@@ -402,3 +402,23 @@ llvm.func @func_debug_directives() {
 llvm.func @class_method() {
   llvm.return loc(#loc3)
 } loc(#loc3)
+
+// -----
+
+// 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, 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<()>
+
+// 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]]}