[Mlir-commits] [mlir] [MLIR][LLVM] Recursion importer handle repeated self-references (PR #87295)

Billy Zhu llvmlistbot at llvm.org
Thu Apr 4 13:49:22 PDT 2024


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

>From 01fd161b6e26f36261ee1076ec8b625e233aecbe Mon Sep 17 00:00:00 2001
From: Billy Zhu <billyzhu at modular.com>
Date: Mon, 1 Apr 2024 14:56:46 -0700
Subject: [PATCH 01/10] create translation helper

---
 mlir/lib/Target/LLVMIR/DebugImporter.cpp     | 331 +++++++++++++++----
 mlir/lib/Target/LLVMIR/DebugImporter.h       |  20 +-
 mlir/test/Target/LLVMIR/Import/debug-info.ll |  41 +++
 3 files changed, 320 insertions(+), 72 deletions(-)

diff --git a/mlir/lib/Target/LLVMIR/DebugImporter.cpp b/mlir/lib/Target/LLVMIR/DebugImporter.cpp
index 779ad26fc847e6..3de62d960ffc44 100644
--- a/mlir/lib/Target/LLVMIR/DebugImporter.cpp
+++ b/mlir/lib/Target/LLVMIR/DebugImporter.cpp
@@ -25,6 +25,269 @@ using namespace mlir;
 using namespace mlir::LLVM;
 using namespace mlir::LLVM::detail;
 
+/// Get the `getRecSelf` constructor for the translated type of `node` if its
+/// translated DITypeAttr supports recursion. Otherwise, returns nullptr.
+static function_ref<DIRecursiveTypeAttrInterface(DistinctAttr)>
+getRecSelfConstructor(llvm::DINode *node) {
+  using CtorType = function_ref<DIRecursiveTypeAttrInterface(DistinctAttr)>;
+  return TypeSwitch<llvm::DINode *, CtorType>(node)
+      .Case([&](llvm::DICompositeType *) {
+        return CtorType(DICompositeTypeAttr::getRecSelf);
+      })
+      .Default(CtorType());
+}
+
+/// Translation helper for recursive DINodes.
+/// Works alongside a stack-based DINode translator (the "main translator") for
+/// gracefully handling DINodes that are recursive.
+///
+/// Usage:
+/// - Before translating a node, call `tryPrune` to see if the pruner can
+///   preempt this translation. If this is a node that the pruner already knows
+///   how to handle, it will return the translated DINodeAttr.
+/// - After a node is successfully translated by the main translator, call
+///   `finalizeTranslation` to save the translated result with the pruner, and
+///   give it a chance to further modify the result.
+/// - Regardless of success or failure by the main translator, always call
+///   `finally` at the end of translating a node. This is necessary to keep the
+///   internal book-keeping in sync.
+///
+/// This helper maintains an internal cache so that no recursive type will
+/// be translated more than once by the main translator.
+/// This internal cache is different from the cache maintained by the main
+/// translator because it may store nodes that are not self-contained (i.e.
+/// contain unbounded recursive self-references).
+class mlir::LLVM::detail::RecursionPruner {
+public:
+  RecursionPruner(MLIRContext *context) : context(context) {}
+
+  /// If this node was previously cached, returns the cached result.
+  /// If this node is a recursive instance that was previously seen, returns a
+  /// self-reference.
+  /// Otherwise, returns null attr.
+  DINodeAttr tryPrune(llvm::DINode *node) {
+    // Lookup the cache first.
+    auto [result, unboundSelfRefs] = lookup(node);
+    if (result) {
+      // Need to inject unbound self-refs into the previous layer.
+      if (!unboundSelfRefs.empty())
+        translationStack.back().second.unboundSelfRefs.insert(
+            unboundSelfRefs.begin(), unboundSelfRefs.end());
+      return result;
+    }
+
+    // If the node type is capable of being recursive, check if it's seen
+    // before.
+    auto recSelfCtor = getRecSelfConstructor(node);
+    if (recSelfCtor) {
+      // If a cyclic dependency is detected since the same node is being
+      // traversed twice, emit a recursive self type, and mark the duplicate
+      // node on the translationStack so it can emit a recursive decl type.
+      auto [iter, inserted] = translationStack.try_emplace(node);
+      if (!inserted) {
+        // The original node may have already been assigned a recursive ID from
+        // a different self-reference. Use that if possible.
+        DIRecursiveTypeAttrInterface recSelf = iter->second.recSelf;
+        if (!recSelf) {
+          DistinctAttr recId = DistinctAttr::create(UnitAttr::get(context));
+          recSelf = recSelfCtor(recId);
+          iter->second.recSelf = recSelf;
+        }
+        // Inject the self-ref into the previous layer.
+        translationStack.back().second.unboundSelfRefs.insert(recSelf);
+        return cast<DINodeAttr>(recSelf);
+      }
+    }
+    return nullptr;
+  }
+
+  /// Register the translated result of `node`. Returns the finalized result
+  /// (with recId if recursive) and whether the result is self-contained
+  /// (i.e. contains no unbound self-refs).
+  std::pair<DINodeAttr, bool> finalizeTranslation(llvm::DINode *node,
+                                                  DINodeAttr result) {
+    // If `node` is not a potentially recursive type, it will not be on the
+    // translation stack. Nothing to set in this case.
+    if (translationStack.empty())
+      return {result, true};
+    if (translationStack.back().first != node)
+      return {result, translationStack.back().second.unboundSelfRefs.empty()};
+
+    TranslationState &state = translationStack.back().second;
+
+    // If this node is actually recursive, set the recId onto `result`.
+    if (DIRecursiveTypeAttrInterface recSelf = state.recSelf) {
+      auto recType = cast<DIRecursiveTypeAttrInterface>(result);
+      result = cast<DINodeAttr>(recType.withRecId(recSelf.getRecId()));
+
+      // Remove this recSelf from the set of unbound selfRefs.
+      state.unboundSelfRefs.erase(recSelf);
+
+      // Insert the newly resolved recursive type into the cache entries that
+      // rely on it.
+      // Only need to look at the caches at this level.
+      uint64_t numRemaining = state.cacheSize;
+      for (auto &cacheEntry : llvm::make_range(cache.rbegin(), cache.rend())) {
+        if (numRemaining == 0)
+          break;
+
+        if (auto refIter = cacheEntry.second.pendingReplacements.find(recSelf);
+            refIter != cacheEntry.second.pendingReplacements.end()) {
+          refIter->second = result;
+        }
+
+        --numRemaining;
+      }
+    }
+
+    // Insert the current result into the cache.
+    state.cacheSize++;
+    auto [iter, inserted] = cache.try_emplace(node);
+    assert(inserted && "invalid state: caching the same DINode twice");
+    iter->second.attr = result;
+
+    // If this node had any unbound self-refs free when it is registered into
+    // the cache, set up replacement placeholders: This result will need these
+    // unbound self-refs to be replaced before being used.
+    for (DIRecursiveTypeAttrInterface selfRef : state.unboundSelfRefs)
+      iter->second.pendingReplacements.try_emplace(selfRef, nullptr);
+
+    return {result, state.unboundSelfRefs.empty()};
+  }
+
+  /// Pop off a frame from the translation stack after a node is done being
+  /// translated.
+  void finally(llvm::DINode *node) {
+    // If `node` is not a potentially recursive type, it will not be on the
+    // translation stack. Nothing to handle in this case.
+    if (translationStack.empty() || translationStack.back().first != node)
+      return;
+
+    // At the end of the stack, all unbound self-refs must be resolved already,
+    // and the entire cache should be accounted for.
+    TranslationState &currLayerState = translationStack.back().second;
+    if (translationStack.size() == 1) {
+      assert(currLayerState.unboundSelfRefs.empty() &&
+             "internal error: unbound recursive self reference at top level.");
+      assert(currLayerState.cacheSize == cache.size() &&
+             "internal error: inconsistent cache size");
+      translationStack.pop_back();
+      cache.clear();
+      return;
+    }
+
+    // Copy unboundSelfRefs down to the previous level.
+    TranslationState &nextLayerState = (++translationStack.rbegin())->second;
+    nextLayerState.unboundSelfRefs.insert(
+        currLayerState.unboundSelfRefs.begin(),
+        currLayerState.unboundSelfRefs.end());
+
+    // The current layer cache is now considered part of the lower layer cache.
+    nextLayerState.cacheSize += currLayerState.cacheSize;
+
+    // Finally pop off this layer when all bookkeeping is done.
+    translationStack.pop_back();
+  }
+
+private:
+  /// Returns the cached result (if exists) with all known replacements applied.
+  /// Also returns the set of unbound self-refs that are unresolved in this
+  /// cached result.
+  /// The cache entry will also be updated with the replaced result and with the
+  /// applied replacements removed from the pendingReplacements map.
+  std::pair<DINodeAttr, llvm::DenseSet<DIRecursiveTypeAttrInterface>>
+  lookup(llvm::DINode *node) {
+    auto cacheIter = cache.find(node);
+    if (cacheIter == cache.end())
+      return {};
+
+    CachedTranslation &entry = cacheIter->second;
+
+    if (entry.pendingReplacements.empty())
+      return std::make_pair(entry.attr,
+                            llvm::DenseSet<DIRecursiveTypeAttrInterface>{});
+
+    mlir::AttrTypeReplacer replacer;
+    replacer.addReplacement(
+        [&entry](DIRecursiveTypeAttrInterface attr)
+            -> std::optional<std::pair<Attribute, WalkResult>> {
+          if (auto replacement = entry.pendingReplacements.lookup(attr)) {
+            // A replacement may contain additional unbound self-refs.
+            return std::make_pair(replacement, mlir::WalkResult::advance());
+          }
+          return std::make_pair(attr, mlir::WalkResult::skip());
+        });
+
+    Attribute replacedAttr = replacer.replace(entry.attr);
+    DINodeAttr result = cast<DINodeAttr>(replacedAttr);
+
+    // Update cache entry to save replaced version and remove already-applied
+    // replacements.
+    entry.attr = result;
+    DenseSet<DIRecursiveTypeAttrInterface> unboundRefs;
+    DenseSet<DIRecursiveTypeAttrInterface> boundRefs;
+    for (auto [refSelf, replacement] : entry.pendingReplacements) {
+      if (replacement)
+        boundRefs.insert(refSelf);
+      else
+        unboundRefs.insert(refSelf);
+    }
+
+    for (DIRecursiveTypeAttrInterface ref : boundRefs)
+      entry.pendingReplacements.erase(ref);
+
+    return std::make_pair(result, unboundRefs);
+  }
+
+  MLIRContext *context;
+
+  /// A lazy cached translation that contains the translated attribute as well
+  /// as any unbound self-references that need to be replaced at lookup.
+  struct CachedTranslation {
+    /// The translated attr. May contain unbound self-references for other
+    /// recursive attrs.
+    DINodeAttr attr;
+    /// The pending replacements that need to be run on this `attr` before it
+    /// can be used.
+    /// Each key is a recursive self-ref, and each value is a recursive decl
+    /// that may contain additional unbound self-refs that need to be replaced.
+    /// Each replacement will be applied at most once. Once a replacement is
+    /// applied, the cached `attr` will be updated, and the replacement will be
+    /// removed from this map.
+    DenseMap<DIRecursiveTypeAttrInterface, DINodeAttr> pendingReplacements;
+  };
+  /// A mapping between LLVM debug metadata and the corresponding attribute.
+  llvm::MapVector<llvm::DINode *, CachedTranslation> cache;
+
+  /// Each potentially recursive node will have a TranslationState pushed onto
+  /// the `translationStack` to keep track of whether this node is actually
+  /// recursive (i.e. has self-references inside), and other book-keeping.
+  struct TranslationState {
+    /// The rec-self if this node is indeed a recursive node (i.e. another
+    /// instance of itself is seen while translating it). Null if this node
+    /// has not been seen again deeper in the translation stack.
+    DIRecursiveTypeAttrInterface recSelf;
+    /// The number of cache entries belonging to this layer of the translation
+    /// stack. This corresponds to the last `cacheSize` entries of `cache`.
+    uint64_t cacheSize = 0;
+    /// All the unbound recursive self references in this layer of the
+    /// translation stack.
+    DenseSet<DIRecursiveTypeAttrInterface> unboundSelfRefs;
+  };
+  /// A stack that stores the metadata nodes that are being traversed. The stack
+  /// is used to handle cyclic dependencies during metadata translation.
+  /// Each node is pushed with an empty TranslationState. If it is ever seen
+  /// later when the stack is deeper, the node is recursive, and its
+  /// TranslationState is assigned a recSelf.
+  llvm::MapVector<llvm::DINode *, TranslationState> translationStack;
+};
+
+DebugImporter::DebugImporter(ModuleOp mlirModule)
+    : recursionPruner(new RecursionPruner(mlirModule.getContext())),
+      context(mlirModule.getContext()), mlirModule(mlirModule) {}
+
+DebugImporter::~DebugImporter() {}
+
 Location DebugImporter::translateFuncLocation(llvm::Function *func) {
   llvm::DISubprogram *subprogram = func->getSubprogram();
   if (!subprogram)
@@ -246,42 +509,12 @@ DINodeAttr DebugImporter::translate(llvm::DINode *node) {
   if (DINodeAttr attr = nodeToAttr.lookup(node))
     return attr;
 
-  // If the node type is capable of being recursive, check if it's seen before.
-  auto recSelfCtor = getRecSelfConstructor(node);
-  if (recSelfCtor) {
-    // If a cyclic dependency is detected since the same node is being traversed
-    // twice, emit a recursive self type, and mark the duplicate node on the
-    // translationStack so it can emit a recursive decl type.
-    auto [iter, inserted] = translationStack.try_emplace(node, nullptr);
-    if (!inserted) {
-      // The original node may have already been assigned a recursive ID from
-      // a different self-reference. Use that if possible.
-      DistinctAttr recId = iter->second;
-      if (!recId) {
-        recId = DistinctAttr::create(UnitAttr::get(context));
-        iter->second = recId;
-      }
-      unboundRecursiveSelfRefs.back().insert(recId);
-      return cast<DINodeAttr>(recSelfCtor(recId));
-    }
-  }
-
-  unboundRecursiveSelfRefs.emplace_back();
-
-  auto guard = llvm::make_scope_exit([&]() {
-    if (recSelfCtor)
-      translationStack.pop_back();
+  // Register with the recursive translator. If it is seen before, return the
+  // result immediately.
+  if (DINodeAttr attr = recursionPruner->tryPrune(node))
+    return attr;
 
-    // Copy unboundRecursiveSelfRefs down to the previous level.
-    if (unboundRecursiveSelfRefs.size() == 1)
-      assert(unboundRecursiveSelfRefs.back().empty() &&
-             "internal error: unbound recursive self reference at top level.");
-    else
-      unboundRecursiveSelfRefs[unboundRecursiveSelfRefs.size() - 2].insert(
-          unboundRecursiveSelfRefs.back().begin(),
-          unboundRecursiveSelfRefs.back().end());
-    unboundRecursiveSelfRefs.pop_back();
-  });
+  auto guard = llvm::make_scope_exit([&]() { recursionPruner->finally(node); });
 
   // Convert the debug metadata if possible.
   auto translateNode = [this](llvm::DINode *node) -> DINodeAttr {
@@ -318,20 +551,12 @@ DINodeAttr DebugImporter::translate(llvm::DINode *node) {
     return nullptr;
   };
   if (DINodeAttr attr = translateNode(node)) {
-    // If this node was marked as recursive, set its recId.
-    if (auto recType = dyn_cast<DIRecursiveTypeAttrInterface>(attr)) {
-      if (DistinctAttr recId = translationStack.lookup(node)) {
-        attr = cast<DINodeAttr>(recType.withRecId(recId));
-        // Remove the unbound recursive ID from the set of unbound self
-        // references in the translation stack.
-        unboundRecursiveSelfRefs.back().erase(recId);
-      }
-    }
-
+    auto [result, isSelfContained] =
+        recursionPruner->finalizeTranslation(node, attr);
     // Only cache fully self-contained nodes.
-    if (unboundRecursiveSelfRefs.back().empty())
-      nodeToAttr.try_emplace(node, attr);
-    return attr;
+    if (isSelfContained)
+      nodeToAttr.try_emplace(node, result);
+    return result;
   }
   return nullptr;
 }
@@ -394,13 +619,3 @@ DistinctAttr DebugImporter::getOrCreateDistinctID(llvm::DINode *node) {
     id = DistinctAttr::create(UnitAttr::get(context));
   return id;
 }
-
-function_ref<DIRecursiveTypeAttrInterface(DistinctAttr)>
-DebugImporter::getRecSelfConstructor(llvm::DINode *node) {
-  using CtorType = function_ref<DIRecursiveTypeAttrInterface(DistinctAttr)>;
-  return TypeSwitch<llvm::DINode *, CtorType>(node)
-      .Case([&](llvm::DICompositeType *concreteNode) {
-        return CtorType(DICompositeTypeAttr::getRecSelf);
-      })
-      .Default(CtorType());
-}
diff --git a/mlir/lib/Target/LLVMIR/DebugImporter.h b/mlir/lib/Target/LLVMIR/DebugImporter.h
index bcf628fc4234fb..1344362ff8d749 100644
--- a/mlir/lib/Target/LLVMIR/DebugImporter.h
+++ b/mlir/lib/Target/LLVMIR/DebugImporter.h
@@ -27,10 +27,12 @@ class LLVMFuncOp;
 
 namespace detail {
 
+class RecursionPruner;
+
 class DebugImporter {
 public:
-  DebugImporter(ModuleOp mlirModule)
-      : context(mlirModule.getContext()), mlirModule(mlirModule) {}
+  DebugImporter(ModuleOp mlirModule);
+  ~DebugImporter();
 
   /// Translates the given LLVM debug location to an MLIR location.
   Location translateLoc(llvm::DILocation *loc);
@@ -86,24 +88,14 @@ class DebugImporter {
   /// for it, or create a new one if not.
   DistinctAttr getOrCreateDistinctID(llvm::DINode *node);
 
-  /// Get the `getRecSelf` constructor for the translated type of `node` if its
-  /// translated DITypeAttr supports recursion. Otherwise, returns nullptr.
-  function_ref<DIRecursiveTypeAttrInterface(DistinctAttr)>
-  getRecSelfConstructor(llvm::DINode *node);
-
   /// A mapping between LLVM debug metadata and the corresponding attribute.
   DenseMap<llvm::DINode *, DINodeAttr> nodeToAttr;
   /// A mapping between distinct LLVM debug metadata nodes and the corresponding
   /// distinct id attribute.
   DenseMap<llvm::DINode *, DistinctAttr> nodeToDistinctAttr;
 
-  /// A stack that stores the metadata nodes that are being traversed. The stack
-  /// is used to detect cyclic dependencies during the metadata translation.
-  /// A node is pushed with a null value. If it is ever seen twice, it is given
-  /// a recursive id attribute, indicating that it is a recursive node.
-  llvm::MapVector<llvm::DINode *, DistinctAttr> translationStack;
-  /// All the unbound recursive self references in the translation stack.
-  SmallVector<DenseSet<DistinctAttr>> unboundRecursiveSelfRefs;
+  // Translation copilot for recursive types.
+  std::unique_ptr<RecursionPruner> recursionPruner;
 
   MLIRContext *context;
   ModuleOp mlirModule;
diff --git a/mlir/test/Target/LLVMIR/Import/debug-info.ll b/mlir/test/Target/LLVMIR/Import/debug-info.ll
index 959a5a1cd97176..b322840d852611 100644
--- a/mlir/test/Target/LLVMIR/Import/debug-info.ll
+++ b/mlir/test/Target/LLVMIR/Import/debug-info.ll
@@ -607,3 +607,44 @@ declare !dbg !1 void @declaration()
 !0 = !{i32 2, !"Debug Info Version", i32 3}
 !1 = !DISubprogram(name: "declaration", scope: !2, file: !2, flags: DIFlagPrototyped, spFlags: 0)
 !2 = !DIFile(filename: "debug-info.ll", directory: "/")
+
+; // -----
+
+; Ensure that repeated occurence of recursive subtree does not result in duplicate MLIR entries.
+
+; CHECK-DAG: #[[B1_INNER:.+]] = #llvm.di_derived_type<{{.*}}name = "B:B1", baseType = #[[B_SELF:.+]]>
+; CHECK-DAG: #[[B2_INNER:.+]] = #llvm.di_derived_type<{{.*}}name = "B:B2", baseType = #[[B_SELF]]>
+; CHECK-DAG: #[[B_INNER:.+]] = #llvm.di_composite_type<{{.*}}recId = [[B_RECID:.+]], {{.*}}name = "B", {{.*}}elements = #[[B1_INNER]], #[[B2_INNER]]
+
+; CHECK-DAG: #[[B1_OUTER:.+]] = #llvm.di_derived_type<{{.*}}name = "B:B1", baseType = #[[B_INNER]]>
+; CHECK-DAG: #[[B2_OUTER:.+]] = #llvm.di_derived_type<{{.*}}name = "B:B2", baseType = #[[B_INNER]]>
+; CHECK-DAG: #[[A_OUTER:.+]] = #llvm.di_composite_type<{{.*}}recId = [[A_RECID:.+]], {{.*}}name = "A", {{.*}}elements = #[[B1_OUTER]], #[[B2_OUTER]]
+
+; CHECK-DAG: #[[A_SELF:.+]] = #llvm.di_composite_type<{{.*}}recId = [[A_RECID]]
+; CHECK-DAG: #[[B_SELF:.+]] = #llvm.di_composite_type<{{.*}}recId = [[B_RECID]]
+
+; CHECK: #llvm.di_subprogram<{{.*}}scope = #[[A_OUTER]]
+
+
+define void @class_field(ptr %arg1) !dbg !18 {
+  ret void
+}
+
+declare void @llvm.dbg.value(metadata, metadata, metadata)
+
+!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 = !DICompositeType(tag: DW_TAG_class_type, name: "A", file: !2, line: 42, flags: DIFlagTypePassByReference | DIFlagNonTrivial, elements: !4)
+!4 = !{!7, !8}
+
+!5 = !DICompositeType(tag: DW_TAG_class_type, name: "B", scope: !3, file: !2, line: 42, flags: DIFlagTypePassByReference | DIFlagNonTrivial, elements: !9)
+!7 = !DIDerivedType(tag: DW_TAG_member, name: "B:B1", file: !2, baseType: !5)
+!8 = !DIDerivedType(tag: DW_TAG_member, name: "B:B2", file: !2, baseType: !5)
+!9 = !{!7, !8}
+
+!18 = distinct !DISubprogram(name: "A", scope: !3, file: !2, spFlags: DISPFlagDefinition, unit: !1)

>From ee93b95ad5f3aee6d7deba6e1a5bd623502d1947 Mon Sep 17 00:00:00 2001
From: Billy Zhu <billyzhu at modular.com>
Date: Tue, 2 Apr 2024 13:30:43 -0700
Subject: [PATCH 02/10] nit suggestions

---
 mlir/lib/Target/LLVMIR/DebugImporter.cpp     | 22 ++++++++++----------
 mlir/lib/Target/LLVMIR/DebugImporter.h       |  2 +-
 mlir/test/Target/LLVMIR/Import/debug-info.ll |  2 --
 3 files changed, 12 insertions(+), 14 deletions(-)

diff --git a/mlir/lib/Target/LLVMIR/DebugImporter.cpp b/mlir/lib/Target/LLVMIR/DebugImporter.cpp
index 3de62d960ffc44..60c469a03d53a1 100644
--- a/mlir/lib/Target/LLVMIR/DebugImporter.cpp
+++ b/mlir/lib/Target/LLVMIR/DebugImporter.cpp
@@ -127,16 +127,15 @@ class mlir::LLVM::detail::RecursionPruner {
       // rely on it.
       // Only need to look at the caches at this level.
       uint64_t numRemaining = state.cacheSize;
-      for (auto &cacheEntry : llvm::make_range(cache.rbegin(), cache.rend())) {
+      for (CachedTranslation &cacheEntry :
+           llvm::make_second_range(llvm::reverse(cache))) {
         if (numRemaining == 0)
           break;
+        --numRemaining;
 
-        if (auto refIter = cacheEntry.second.pendingReplacements.find(recSelf);
-            refIter != cacheEntry.second.pendingReplacements.end()) {
+        if (auto refIter = cacheEntry.pendingReplacements.find(recSelf);
+            refIter != cacheEntry.pendingReplacements.end())
           refIter->second = result;
-        }
-
-        --numRemaining;
       }
     }
 
@@ -157,7 +156,7 @@ class mlir::LLVM::detail::RecursionPruner {
 
   /// Pop off a frame from the translation stack after a node is done being
   /// translated.
-  void finally(llvm::DINode *node) {
+  void popTranslationStack(llvm::DINode *node) {
     // If `node` is not a potentially recursive type, it will not be on the
     // translation stack. Nothing to handle in this case.
     if (translationStack.empty() || translationStack.back().first != node)
@@ -195,7 +194,7 @@ class mlir::LLVM::detail::RecursionPruner {
   /// cached result.
   /// The cache entry will also be updated with the replaced result and with the
   /// applied replacements removed from the pendingReplacements map.
-  std::pair<DINodeAttr, llvm::DenseSet<DIRecursiveTypeAttrInterface>>
+  std::pair<DINodeAttr, DenseSet<DIRecursiveTypeAttrInterface>>
   lookup(llvm::DINode *node) {
     auto cacheIter = cache.find(node);
     if (cacheIter == cache.end())
@@ -205,9 +204,9 @@ class mlir::LLVM::detail::RecursionPruner {
 
     if (entry.pendingReplacements.empty())
       return std::make_pair(entry.attr,
-                            llvm::DenseSet<DIRecursiveTypeAttrInterface>{});
+                            DenseSet<DIRecursiveTypeAttrInterface>{});
 
-    mlir::AttrTypeReplacer replacer;
+    AttrTypeReplacer replacer;
     replacer.addReplacement(
         [&entry](DIRecursiveTypeAttrInterface attr)
             -> std::optional<std::pair<Attribute, WalkResult>> {
@@ -514,7 +513,8 @@ DINodeAttr DebugImporter::translate(llvm::DINode *node) {
   if (DINodeAttr attr = recursionPruner->tryPrune(node))
     return attr;
 
-  auto guard = llvm::make_scope_exit([&]() { recursionPruner->finally(node); });
+  auto guard = llvm::make_scope_exit(
+      [&]() { recursionPruner->popTranslationStack(node); });
 
   // Convert the debug metadata if possible.
   auto translateNode = [this](llvm::DINode *node) -> DINodeAttr {
diff --git a/mlir/lib/Target/LLVMIR/DebugImporter.h b/mlir/lib/Target/LLVMIR/DebugImporter.h
index 1344362ff8d749..a95346cd3cd9c4 100644
--- a/mlir/lib/Target/LLVMIR/DebugImporter.h
+++ b/mlir/lib/Target/LLVMIR/DebugImporter.h
@@ -94,7 +94,7 @@ class DebugImporter {
   /// distinct id attribute.
   DenseMap<llvm::DINode *, DistinctAttr> nodeToDistinctAttr;
 
-  // Translation copilot for recursive types.
+  /// A translation helper for recursive types.
   std::unique_ptr<RecursionPruner> recursionPruner;
 
   MLIRContext *context;
diff --git a/mlir/test/Target/LLVMIR/Import/debug-info.ll b/mlir/test/Target/LLVMIR/Import/debug-info.ll
index b322840d852611..931f914c995bb6 100644
--- a/mlir/test/Target/LLVMIR/Import/debug-info.ll
+++ b/mlir/test/Target/LLVMIR/Import/debug-info.ll
@@ -625,7 +625,6 @@ declare !dbg !1 void @declaration()
 
 ; CHECK: #llvm.di_subprogram<{{.*}}scope = #[[A_OUTER]]
 
-
 define void @class_field(ptr %arg1) !dbg !18 {
   ret void
 }
@@ -638,7 +637,6 @@ declare void @llvm.dbg.value(metadata, metadata, metadata)
 !1 = distinct !DICompileUnit(language: DW_LANG_C, file: !2)
 !2 = !DIFile(filename: "debug-info.ll", directory: "/")
 
-
 !3 = !DICompositeType(tag: DW_TAG_class_type, name: "A", file: !2, line: 42, flags: DIFlagTypePassByReference | DIFlagNonTrivial, elements: !4)
 !4 = !{!7, !8}
 

>From 5f01209001046325101bb2e57b07d5a258a9c3f9 Mon Sep 17 00:00:00 2001
From: Billy Zhu <billyzhu at modular.com>
Date: Tue, 2 Apr 2024 13:37:27 -0700
Subject: [PATCH 03/10] move decl into header and make direct member

---
 mlir/lib/Target/LLVMIR/DebugImporter.cpp | 450 ++++++++++-------------
 mlir/lib/Target/LLVMIR/DebugImporter.h   |  96 ++++-
 2 files changed, 282 insertions(+), 264 deletions(-)

diff --git a/mlir/lib/Target/LLVMIR/DebugImporter.cpp b/mlir/lib/Target/LLVMIR/DebugImporter.cpp
index 60c469a03d53a1..f57411dc7d97c0 100644
--- a/mlir/lib/Target/LLVMIR/DebugImporter.cpp
+++ b/mlir/lib/Target/LLVMIR/DebugImporter.cpp
@@ -25,264 +25,8 @@ using namespace mlir;
 using namespace mlir::LLVM;
 using namespace mlir::LLVM::detail;
 
-/// Get the `getRecSelf` constructor for the translated type of `node` if its
-/// translated DITypeAttr supports recursion. Otherwise, returns nullptr.
-static function_ref<DIRecursiveTypeAttrInterface(DistinctAttr)>
-getRecSelfConstructor(llvm::DINode *node) {
-  using CtorType = function_ref<DIRecursiveTypeAttrInterface(DistinctAttr)>;
-  return TypeSwitch<llvm::DINode *, CtorType>(node)
-      .Case([&](llvm::DICompositeType *) {
-        return CtorType(DICompositeTypeAttr::getRecSelf);
-      })
-      .Default(CtorType());
-}
-
-/// Translation helper for recursive DINodes.
-/// Works alongside a stack-based DINode translator (the "main translator") for
-/// gracefully handling DINodes that are recursive.
-///
-/// Usage:
-/// - Before translating a node, call `tryPrune` to see if the pruner can
-///   preempt this translation. If this is a node that the pruner already knows
-///   how to handle, it will return the translated DINodeAttr.
-/// - After a node is successfully translated by the main translator, call
-///   `finalizeTranslation` to save the translated result with the pruner, and
-///   give it a chance to further modify the result.
-/// - Regardless of success or failure by the main translator, always call
-///   `finally` at the end of translating a node. This is necessary to keep the
-///   internal book-keeping in sync.
-///
-/// This helper maintains an internal cache so that no recursive type will
-/// be translated more than once by the main translator.
-/// This internal cache is different from the cache maintained by the main
-/// translator because it may store nodes that are not self-contained (i.e.
-/// contain unbounded recursive self-references).
-class mlir::LLVM::detail::RecursionPruner {
-public:
-  RecursionPruner(MLIRContext *context) : context(context) {}
-
-  /// If this node was previously cached, returns the cached result.
-  /// If this node is a recursive instance that was previously seen, returns a
-  /// self-reference.
-  /// Otherwise, returns null attr.
-  DINodeAttr tryPrune(llvm::DINode *node) {
-    // Lookup the cache first.
-    auto [result, unboundSelfRefs] = lookup(node);
-    if (result) {
-      // Need to inject unbound self-refs into the previous layer.
-      if (!unboundSelfRefs.empty())
-        translationStack.back().second.unboundSelfRefs.insert(
-            unboundSelfRefs.begin(), unboundSelfRefs.end());
-      return result;
-    }
-
-    // If the node type is capable of being recursive, check if it's seen
-    // before.
-    auto recSelfCtor = getRecSelfConstructor(node);
-    if (recSelfCtor) {
-      // If a cyclic dependency is detected since the same node is being
-      // traversed twice, emit a recursive self type, and mark the duplicate
-      // node on the translationStack so it can emit a recursive decl type.
-      auto [iter, inserted] = translationStack.try_emplace(node);
-      if (!inserted) {
-        // The original node may have already been assigned a recursive ID from
-        // a different self-reference. Use that if possible.
-        DIRecursiveTypeAttrInterface recSelf = iter->second.recSelf;
-        if (!recSelf) {
-          DistinctAttr recId = DistinctAttr::create(UnitAttr::get(context));
-          recSelf = recSelfCtor(recId);
-          iter->second.recSelf = recSelf;
-        }
-        // Inject the self-ref into the previous layer.
-        translationStack.back().second.unboundSelfRefs.insert(recSelf);
-        return cast<DINodeAttr>(recSelf);
-      }
-    }
-    return nullptr;
-  }
-
-  /// Register the translated result of `node`. Returns the finalized result
-  /// (with recId if recursive) and whether the result is self-contained
-  /// (i.e. contains no unbound self-refs).
-  std::pair<DINodeAttr, bool> finalizeTranslation(llvm::DINode *node,
-                                                  DINodeAttr result) {
-    // If `node` is not a potentially recursive type, it will not be on the
-    // translation stack. Nothing to set in this case.
-    if (translationStack.empty())
-      return {result, true};
-    if (translationStack.back().first != node)
-      return {result, translationStack.back().second.unboundSelfRefs.empty()};
-
-    TranslationState &state = translationStack.back().second;
-
-    // If this node is actually recursive, set the recId onto `result`.
-    if (DIRecursiveTypeAttrInterface recSelf = state.recSelf) {
-      auto recType = cast<DIRecursiveTypeAttrInterface>(result);
-      result = cast<DINodeAttr>(recType.withRecId(recSelf.getRecId()));
-
-      // Remove this recSelf from the set of unbound selfRefs.
-      state.unboundSelfRefs.erase(recSelf);
-
-      // Insert the newly resolved recursive type into the cache entries that
-      // rely on it.
-      // Only need to look at the caches at this level.
-      uint64_t numRemaining = state.cacheSize;
-      for (CachedTranslation &cacheEntry :
-           llvm::make_second_range(llvm::reverse(cache))) {
-        if (numRemaining == 0)
-          break;
-        --numRemaining;
-
-        if (auto refIter = cacheEntry.pendingReplacements.find(recSelf);
-            refIter != cacheEntry.pendingReplacements.end())
-          refIter->second = result;
-      }
-    }
-
-    // Insert the current result into the cache.
-    state.cacheSize++;
-    auto [iter, inserted] = cache.try_emplace(node);
-    assert(inserted && "invalid state: caching the same DINode twice");
-    iter->second.attr = result;
-
-    // If this node had any unbound self-refs free when it is registered into
-    // the cache, set up replacement placeholders: This result will need these
-    // unbound self-refs to be replaced before being used.
-    for (DIRecursiveTypeAttrInterface selfRef : state.unboundSelfRefs)
-      iter->second.pendingReplacements.try_emplace(selfRef, nullptr);
-
-    return {result, state.unboundSelfRefs.empty()};
-  }
-
-  /// Pop off a frame from the translation stack after a node is done being
-  /// translated.
-  void popTranslationStack(llvm::DINode *node) {
-    // If `node` is not a potentially recursive type, it will not be on the
-    // translation stack. Nothing to handle in this case.
-    if (translationStack.empty() || translationStack.back().first != node)
-      return;
-
-    // At the end of the stack, all unbound self-refs must be resolved already,
-    // and the entire cache should be accounted for.
-    TranslationState &currLayerState = translationStack.back().second;
-    if (translationStack.size() == 1) {
-      assert(currLayerState.unboundSelfRefs.empty() &&
-             "internal error: unbound recursive self reference at top level.");
-      assert(currLayerState.cacheSize == cache.size() &&
-             "internal error: inconsistent cache size");
-      translationStack.pop_back();
-      cache.clear();
-      return;
-    }
-
-    // Copy unboundSelfRefs down to the previous level.
-    TranslationState &nextLayerState = (++translationStack.rbegin())->second;
-    nextLayerState.unboundSelfRefs.insert(
-        currLayerState.unboundSelfRefs.begin(),
-        currLayerState.unboundSelfRefs.end());
-
-    // The current layer cache is now considered part of the lower layer cache.
-    nextLayerState.cacheSize += currLayerState.cacheSize;
-
-    // Finally pop off this layer when all bookkeeping is done.
-    translationStack.pop_back();
-  }
-
-private:
-  /// Returns the cached result (if exists) with all known replacements applied.
-  /// Also returns the set of unbound self-refs that are unresolved in this
-  /// cached result.
-  /// The cache entry will also be updated with the replaced result and with the
-  /// applied replacements removed from the pendingReplacements map.
-  std::pair<DINodeAttr, DenseSet<DIRecursiveTypeAttrInterface>>
-  lookup(llvm::DINode *node) {
-    auto cacheIter = cache.find(node);
-    if (cacheIter == cache.end())
-      return {};
-
-    CachedTranslation &entry = cacheIter->second;
-
-    if (entry.pendingReplacements.empty())
-      return std::make_pair(entry.attr,
-                            DenseSet<DIRecursiveTypeAttrInterface>{});
-
-    AttrTypeReplacer replacer;
-    replacer.addReplacement(
-        [&entry](DIRecursiveTypeAttrInterface attr)
-            -> std::optional<std::pair<Attribute, WalkResult>> {
-          if (auto replacement = entry.pendingReplacements.lookup(attr)) {
-            // A replacement may contain additional unbound self-refs.
-            return std::make_pair(replacement, mlir::WalkResult::advance());
-          }
-          return std::make_pair(attr, mlir::WalkResult::skip());
-        });
-
-    Attribute replacedAttr = replacer.replace(entry.attr);
-    DINodeAttr result = cast<DINodeAttr>(replacedAttr);
-
-    // Update cache entry to save replaced version and remove already-applied
-    // replacements.
-    entry.attr = result;
-    DenseSet<DIRecursiveTypeAttrInterface> unboundRefs;
-    DenseSet<DIRecursiveTypeAttrInterface> boundRefs;
-    for (auto [refSelf, replacement] : entry.pendingReplacements) {
-      if (replacement)
-        boundRefs.insert(refSelf);
-      else
-        unboundRefs.insert(refSelf);
-    }
-
-    for (DIRecursiveTypeAttrInterface ref : boundRefs)
-      entry.pendingReplacements.erase(ref);
-
-    return std::make_pair(result, unboundRefs);
-  }
-
-  MLIRContext *context;
-
-  /// A lazy cached translation that contains the translated attribute as well
-  /// as any unbound self-references that need to be replaced at lookup.
-  struct CachedTranslation {
-    /// The translated attr. May contain unbound self-references for other
-    /// recursive attrs.
-    DINodeAttr attr;
-    /// The pending replacements that need to be run on this `attr` before it
-    /// can be used.
-    /// Each key is a recursive self-ref, and each value is a recursive decl
-    /// that may contain additional unbound self-refs that need to be replaced.
-    /// Each replacement will be applied at most once. Once a replacement is
-    /// applied, the cached `attr` will be updated, and the replacement will be
-    /// removed from this map.
-    DenseMap<DIRecursiveTypeAttrInterface, DINodeAttr> pendingReplacements;
-  };
-  /// A mapping between LLVM debug metadata and the corresponding attribute.
-  llvm::MapVector<llvm::DINode *, CachedTranslation> cache;
-
-  /// Each potentially recursive node will have a TranslationState pushed onto
-  /// the `translationStack` to keep track of whether this node is actually
-  /// recursive (i.e. has self-references inside), and other book-keeping.
-  struct TranslationState {
-    /// The rec-self if this node is indeed a recursive node (i.e. another
-    /// instance of itself is seen while translating it). Null if this node
-    /// has not been seen again deeper in the translation stack.
-    DIRecursiveTypeAttrInterface recSelf;
-    /// The number of cache entries belonging to this layer of the translation
-    /// stack. This corresponds to the last `cacheSize` entries of `cache`.
-    uint64_t cacheSize = 0;
-    /// All the unbound recursive self references in this layer of the
-    /// translation stack.
-    DenseSet<DIRecursiveTypeAttrInterface> unboundSelfRefs;
-  };
-  /// A stack that stores the metadata nodes that are being traversed. The stack
-  /// is used to handle cyclic dependencies during metadata translation.
-  /// Each node is pushed with an empty TranslationState. If it is ever seen
-  /// later when the stack is deeper, the node is recursive, and its
-  /// TranslationState is assigned a recSelf.
-  llvm::MapVector<llvm::DINode *, TranslationState> translationStack;
-};
-
 DebugImporter::DebugImporter(ModuleOp mlirModule)
-    : recursionPruner(new RecursionPruner(mlirModule.getContext())),
+    : recursionPruner(mlirModule.getContext()),
       context(mlirModule.getContext()), mlirModule(mlirModule) {}
 
 DebugImporter::~DebugImporter() {}
@@ -510,11 +254,11 @@ DINodeAttr DebugImporter::translate(llvm::DINode *node) {
 
   // Register with the recursive translator. If it is seen before, return the
   // result immediately.
-  if (DINodeAttr attr = recursionPruner->tryPrune(node))
+  if (DINodeAttr attr = recursionPruner.tryPrune(node))
     return attr;
 
   auto guard = llvm::make_scope_exit(
-      [&]() { recursionPruner->popTranslationStack(node); });
+      [&]() { recursionPruner.popTranslationStack(node); });
 
   // Convert the debug metadata if possible.
   auto translateNode = [this](llvm::DINode *node) -> DINodeAttr {
@@ -552,7 +296,7 @@ DINodeAttr DebugImporter::translate(llvm::DINode *node) {
   };
   if (DINodeAttr attr = translateNode(node)) {
     auto [result, isSelfContained] =
-        recursionPruner->finalizeTranslation(node, attr);
+        recursionPruner.finalizeTranslation(node, attr);
     // Only cache fully self-contained nodes.
     if (isSelfContained)
       nodeToAttr.try_emplace(node, result);
@@ -561,6 +305,192 @@ DINodeAttr DebugImporter::translate(llvm::DINode *node) {
   return nullptr;
 }
 
+//===----------------------------------------------------------------------===//
+// RecursionPruner
+//===----------------------------------------------------------------------===//
+/// Get the `getRecSelf` constructor for the translated type of `node` if its
+/// translated DITypeAttr supports recursion. Otherwise, returns nullptr.
+static function_ref<DIRecursiveTypeAttrInterface(DistinctAttr)>
+getRecSelfConstructor(llvm::DINode *node) {
+  using CtorType = function_ref<DIRecursiveTypeAttrInterface(DistinctAttr)>;
+  return TypeSwitch<llvm::DINode *, CtorType>(node)
+      .Case([&](llvm::DICompositeType *) {
+        return CtorType(DICompositeTypeAttr::getRecSelf);
+      })
+      .Default(CtorType());
+}
+
+DINodeAttr DebugImporter::RecursionPruner::tryPrune(llvm::DINode *node) {
+  // Lookup the cache first.
+  auto [result, unboundSelfRefs] = lookup(node);
+  if (result) {
+    // Need to inject unbound self-refs into the previous layer.
+    if (!unboundSelfRefs.empty())
+      translationStack.back().second.unboundSelfRefs.insert(
+          unboundSelfRefs.begin(), unboundSelfRefs.end());
+    return result;
+  }
+
+  // If the node type is capable of being recursive, check if it's seen
+  // before.
+  auto recSelfCtor = getRecSelfConstructor(node);
+  if (recSelfCtor) {
+    // If a cyclic dependency is detected since the same node is being
+    // traversed twice, emit a recursive self type, and mark the duplicate
+    // node on the translationStack so it can emit a recursive decl type.
+    auto [iter, inserted] = translationStack.try_emplace(node);
+    if (!inserted) {
+      // The original node may have already been assigned a recursive ID from
+      // a different self-reference. Use that if possible.
+      DIRecursiveTypeAttrInterface recSelf = iter->second.recSelf;
+      if (!recSelf) {
+        DistinctAttr recId = DistinctAttr::create(UnitAttr::get(context));
+        recSelf = recSelfCtor(recId);
+        iter->second.recSelf = recSelf;
+      }
+      // Inject the self-ref into the previous layer.
+      translationStack.back().second.unboundSelfRefs.insert(recSelf);
+      return cast<DINodeAttr>(recSelf);
+    }
+  }
+  return nullptr;
+}
+
+/// Register the translated result of `node`. Returns the finalized result
+/// (with recId if recursive) and whether the result is self-contained
+/// (i.e. contains no unbound self-refs).
+std::pair<DINodeAttr, bool>
+DebugImporter::RecursionPruner::finalizeTranslation(llvm::DINode *node,
+                                                    DINodeAttr result) {
+  // If `node` is not a potentially recursive type, it will not be on the
+  // translation stack. Nothing to set in this case.
+  if (translationStack.empty())
+    return {result, true};
+  if (translationStack.back().first != node)
+    return {result, translationStack.back().second.unboundSelfRefs.empty()};
+
+  TranslationState &state = translationStack.back().second;
+
+  // If this node is actually recursive, set the recId onto `result`.
+  if (DIRecursiveTypeAttrInterface recSelf = state.recSelf) {
+    auto recType = cast<DIRecursiveTypeAttrInterface>(result);
+    result = cast<DINodeAttr>(recType.withRecId(recSelf.getRecId()));
+
+    // Remove this recSelf from the set of unbound selfRefs.
+    state.unboundSelfRefs.erase(recSelf);
+
+    // Insert the newly resolved recursive type into the cache entries that
+    // rely on it.
+    // Only need to look at the caches at this level.
+    uint64_t numRemaining = state.cacheSize;
+    for (CachedTranslation &cacheEntry :
+         llvm::make_second_range(llvm::reverse(cache))) {
+      if (numRemaining == 0)
+        break;
+      --numRemaining;
+
+      if (auto refIter = cacheEntry.pendingReplacements.find(recSelf);
+          refIter != cacheEntry.pendingReplacements.end())
+        refIter->second = result;
+    }
+  }
+
+  // Insert the current result into the cache.
+  state.cacheSize++;
+  auto [iter, inserted] = cache.try_emplace(node);
+  assert(inserted && "invalid state: caching the same DINode twice");
+  iter->second.attr = result;
+
+  // If this node had any unbound self-refs free when it is registered into
+  // the cache, set up replacement placeholders: This result will need these
+  // unbound self-refs to be replaced before being used.
+  for (DIRecursiveTypeAttrInterface selfRef : state.unboundSelfRefs)
+    iter->second.pendingReplacements.try_emplace(selfRef, nullptr);
+
+  return {result, state.unboundSelfRefs.empty()};
+}
+
+/// Pop off a frame from the translation stack after a node is done being
+/// translated.
+void DebugImporter::RecursionPruner::popTranslationStack(llvm::DINode *node) {
+  // If `node` is not a potentially recursive type, it will not be on the
+  // translation stack. Nothing to handle in this case.
+  if (translationStack.empty() || translationStack.back().first != node)
+    return;
+
+  // At the end of the stack, all unbound self-refs must be resolved already,
+  // and the entire cache should be accounted for.
+  TranslationState &currLayerState = translationStack.back().second;
+  if (translationStack.size() == 1) {
+    assert(currLayerState.unboundSelfRefs.empty() &&
+           "internal error: unbound recursive self reference at top level.");
+    assert(currLayerState.cacheSize == cache.size() &&
+           "internal error: inconsistent cache size");
+    translationStack.pop_back();
+    cache.clear();
+    return;
+  }
+
+  // Copy unboundSelfRefs down to the previous level.
+  TranslationState &nextLayerState = (++translationStack.rbegin())->second;
+  nextLayerState.unboundSelfRefs.insert(currLayerState.unboundSelfRefs.begin(),
+                                        currLayerState.unboundSelfRefs.end());
+
+  // The current layer cache is now considered part of the lower layer cache.
+  nextLayerState.cacheSize += currLayerState.cacheSize;
+
+  // Finally pop off this layer when all bookkeeping is done.
+  translationStack.pop_back();
+}
+
+/// Returns the cached result (if exists) with all known replacements applied.
+/// Also returns the set of unbound self-refs that are unresolved in this
+/// cached result.
+/// The cache entry will also be updated with the replaced result and with the
+/// applied replacements removed from the pendingReplacements map.
+std::pair<DINodeAttr, DenseSet<DIRecursiveTypeAttrInterface>>
+DebugImporter::RecursionPruner::lookup(llvm::DINode *node) {
+  auto cacheIter = cache.find(node);
+  if (cacheIter == cache.end())
+    return {};
+
+  CachedTranslation &entry = cacheIter->second;
+
+  if (entry.pendingReplacements.empty())
+    return std::make_pair(entry.attr, DenseSet<DIRecursiveTypeAttrInterface>{});
+
+  AttrTypeReplacer replacer;
+  replacer.addReplacement(
+      [&entry](DIRecursiveTypeAttrInterface attr)
+          -> std::optional<std::pair<Attribute, WalkResult>> {
+        if (auto replacement = entry.pendingReplacements.lookup(attr)) {
+          // A replacement may contain additional unbound self-refs.
+          return std::make_pair(replacement, mlir::WalkResult::advance());
+        }
+        return std::make_pair(attr, mlir::WalkResult::skip());
+      });
+
+  Attribute replacedAttr = replacer.replace(entry.attr);
+  DINodeAttr result = cast<DINodeAttr>(replacedAttr);
+
+  // Update cache entry to save replaced version and remove already-applied
+  // replacements.
+  entry.attr = result;
+  DenseSet<DIRecursiveTypeAttrInterface> unboundRefs;
+  DenseSet<DIRecursiveTypeAttrInterface> boundRefs;
+  for (auto [refSelf, replacement] : entry.pendingReplacements) {
+    if (replacement)
+      boundRefs.insert(refSelf);
+    else
+      unboundRefs.insert(refSelf);
+  }
+
+  for (DIRecursiveTypeAttrInterface ref : boundRefs)
+    entry.pendingReplacements.erase(ref);
+
+  return std::make_pair(result, unboundRefs);
+}
+
 //===----------------------------------------------------------------------===//
 // Locations
 //===----------------------------------------------------------------------===//
diff --git a/mlir/lib/Target/LLVMIR/DebugImporter.h b/mlir/lib/Target/LLVMIR/DebugImporter.h
index a95346cd3cd9c4..dd5aba9169dbd1 100644
--- a/mlir/lib/Target/LLVMIR/DebugImporter.h
+++ b/mlir/lib/Target/LLVMIR/DebugImporter.h
@@ -27,8 +27,6 @@ class LLVMFuncOp;
 
 namespace detail {
 
-class RecursionPruner;
-
 class DebugImporter {
 public:
   DebugImporter(ModuleOp mlirModule);
@@ -94,8 +92,98 @@ class DebugImporter {
   /// distinct id attribute.
   DenseMap<llvm::DINode *, DistinctAttr> nodeToDistinctAttr;
 
-  /// A translation helper for recursive types.
-  std::unique_ptr<RecursionPruner> recursionPruner;
+  /// Translation helper for recursive DINodes.
+  /// Works alongside a stack-based DINode translator (the "main translator")
+  /// for gracefully handling DINodes that are recursive.
+  ///
+  /// Usage:
+  /// - Before translating a node, call `tryPrune` to see if the pruner can
+  ///   preempt this translation. If this is a node that the pruner already
+  ///   knows how to handle, it will return the translated DINodeAttr.
+  /// - After a node is successfully translated by the main translator, call
+  ///   `finalizeTranslation` to save the translated result with the pruner, and
+  ///   give it a chance to further modify the result.
+  /// - Regardless of success or failure by the main translator, always call
+  ///   `finally` at the end of translating a node. This is necessary to keep
+  ///   the internal book-keeping in sync.
+  ///
+  /// This helper maintains an internal cache so that no recursive type will
+  /// be translated more than once by the main translator.
+  /// This internal cache is different from the cache maintained by the main
+  /// translator because it may store nodes that are not self-contained (i.e.
+  /// contain unbounded recursive self-references).
+  class RecursionPruner {
+  public:
+    RecursionPruner(MLIRContext *context) : context(context) {}
+
+    /// If this node was previously cached, returns the cached result.
+    /// If this node is a recursive instance that was previously seen, returns a
+    /// self-reference.
+    /// Otherwise, returns null attr.
+    DINodeAttr tryPrune(llvm::DINode *node);
+
+    /// Register the translated result of `node`. Returns the finalized result
+    /// (with recId if recursive) and whether the result is self-contained
+    /// (i.e. contains no unbound self-refs).
+    std::pair<DINodeAttr, bool> finalizeTranslation(llvm::DINode *node,
+                                                    DINodeAttr result);
+
+    /// Pop off a frame from the translation stack after a node is done being
+    /// translated.
+    void popTranslationStack(llvm::DINode *node);
+
+  private:
+    /// Returns the cached result (if exists) with all known replacements
+    /// applied. Also returns the set of unbound self-refs that are unresolved
+    /// in this cached result. The cache entry will also be updated with the
+    /// replaced result and with the applied replacements removed from the
+    /// pendingReplacements map.
+    std::pair<DINodeAttr, DenseSet<DIRecursiveTypeAttrInterface>>
+    lookup(llvm::DINode *node);
+
+    MLIRContext *context;
+
+    /// A lazy cached translation that contains the translated attribute as well
+    /// as any unbound self-references that need to be replaced at lookup.
+    struct CachedTranslation {
+      /// The translated attr. May contain unbound self-references for other
+      /// recursive attrs.
+      DINodeAttr attr;
+      /// The pending replacements that need to be run on this `attr` before it
+      /// can be used.
+      /// Each key is a recursive self-ref, and each value is a recursive decl
+      /// that may contain additional unbound self-refs that need to be
+      /// replaced. Each replacement will be applied at most once. Once a
+      /// replacement is applied, the cached `attr` will be updated, and the
+      /// replacement will be removed from this map.
+      DenseMap<DIRecursiveTypeAttrInterface, DINodeAttr> pendingReplacements;
+    };
+    /// A mapping between LLVM debug metadata and the corresponding attribute.
+    llvm::MapVector<llvm::DINode *, CachedTranslation> cache;
+
+    /// Each potentially recursive node will have a TranslationState pushed onto
+    /// the `translationStack` to keep track of whether this node is actually
+    /// recursive (i.e. has self-references inside), and other book-keeping.
+    struct TranslationState {
+      /// The rec-self if this node is indeed a recursive node (i.e. another
+      /// instance of itself is seen while translating it). Null if this node
+      /// has not been seen again deeper in the translation stack.
+      DIRecursiveTypeAttrInterface recSelf;
+      /// The number of cache entries belonging to this layer of the translation
+      /// stack. This corresponds to the last `cacheSize` entries of `cache`.
+      uint64_t cacheSize = 0;
+      /// All the unbound recursive self references in this layer of the
+      /// translation stack.
+      DenseSet<DIRecursiveTypeAttrInterface> unboundSelfRefs;
+    };
+    /// A stack that stores the metadata nodes that are being traversed. The
+    /// stack is used to handle cyclic dependencies during metadata translation.
+    /// Each node is pushed with an empty TranslationState. If it is ever seen
+    /// later when the stack is deeper, the node is recursive, and its
+    /// TranslationState is assigned a recSelf.
+    llvm::MapVector<llvm::DINode *, TranslationState> translationStack;
+  };
+  RecursionPruner recursionPruner;
 
   MLIRContext *context;
   ModuleOp mlirModule;

>From ffe56df7537a33f6c6c69938369a174c7e33c8a4 Mon Sep 17 00:00:00 2001
From: Billy Zhu <billyzhu at modular.com>
Date: Tue, 2 Apr 2024 13:39:50 -0700
Subject: [PATCH 04/10] rename methods for clarity

---
 mlir/lib/Target/LLVMIR/DebugImporter.cpp |  9 +++++----
 mlir/lib/Target/LLVMIR/DebugImporter.h   | 17 ++++++++++-------
 2 files changed, 15 insertions(+), 11 deletions(-)

diff --git a/mlir/lib/Target/LLVMIR/DebugImporter.cpp b/mlir/lib/Target/LLVMIR/DebugImporter.cpp
index f57411dc7d97c0..f4329dda2c4388 100644
--- a/mlir/lib/Target/LLVMIR/DebugImporter.cpp
+++ b/mlir/lib/Target/LLVMIR/DebugImporter.cpp
@@ -252,9 +252,9 @@ DINodeAttr DebugImporter::translate(llvm::DINode *node) {
   if (DINodeAttr attr = nodeToAttr.lookup(node))
     return attr;
 
-  // Register with the recursive translator. If it is seen before, return the
-  // result immediately.
-  if (DINodeAttr attr = recursionPruner.tryPrune(node))
+  // Register with the recursive translator. If it can be handled without
+  // recursing into it, return the result immediately.
+  if (DINodeAttr attr = recursionPruner.pruneOrPushTranslationStack(node))
     return attr;
 
   auto guard = llvm::make_scope_exit(
@@ -320,7 +320,8 @@ getRecSelfConstructor(llvm::DINode *node) {
       .Default(CtorType());
 }
 
-DINodeAttr DebugImporter::RecursionPruner::tryPrune(llvm::DINode *node) {
+DINodeAttr DebugImporter::RecursionPruner::pruneOrPushTranslationStack(
+    llvm::DINode *node) {
   // Lookup the cache first.
   auto [result, unboundSelfRefs] = lookup(node);
   if (result) {
diff --git a/mlir/lib/Target/LLVMIR/DebugImporter.h b/mlir/lib/Target/LLVMIR/DebugImporter.h
index dd5aba9169dbd1..7f21b836ac9f38 100644
--- a/mlir/lib/Target/LLVMIR/DebugImporter.h
+++ b/mlir/lib/Target/LLVMIR/DebugImporter.h
@@ -97,15 +97,16 @@ class DebugImporter {
   /// for gracefully handling DINodes that are recursive.
   ///
   /// Usage:
-  /// - Before translating a node, call `tryPrune` to see if the pruner can
-  ///   preempt this translation. If this is a node that the pruner already
-  ///   knows how to handle, it will return the translated DINodeAttr.
+  /// - Before translating a node, call `pruneOrPushTranslationStack` to see if
+  ///   the pruner can preempt this translation. If this is a node that the
+  ///   pruner already knows how to handle, it will return the translated
+  ///   DINodeAttr.
   /// - After a node is successfully translated by the main translator, call
   ///   `finalizeTranslation` to save the translated result with the pruner, and
   ///   give it a chance to further modify the result.
   /// - Regardless of success or failure by the main translator, always call
-  ///   `finally` at the end of translating a node. This is necessary to keep
-  ///   the internal book-keeping in sync.
+  ///   `popTranslationStack` at the end of translating a node. This is
+  ///   necessary to keep the internal book-keeping in sync.
   ///
   /// This helper maintains an internal cache so that no recursive type will
   /// be translated more than once by the main translator.
@@ -119,8 +120,10 @@ class DebugImporter {
     /// If this node was previously cached, returns the cached result.
     /// If this node is a recursive instance that was previously seen, returns a
     /// self-reference.
-    /// Otherwise, returns null attr.
-    DINodeAttr tryPrune(llvm::DINode *node);
+    /// Otherwise, returns null attr, and a translation stack frame is created
+    /// for this node. Expects `finalizeTranslation` & `popTranslationStack`
+    /// to be called on this node later.
+    DINodeAttr pruneOrPushTranslationStack(llvm::DINode *node);
 
     /// Register the translated result of `node`. Returns the finalized result
     /// (with recId if recursive) and whether the result is self-contained

>From 3e369fdd51acc41689e0c6f1577206418af6e999 Mon Sep 17 00:00:00 2001
From: Billy Zhu <billyzhu at modular.com>
Date: Tue, 2 Apr 2024 15:41:32 -0700
Subject: [PATCH 05/10] recurse into decl and add test for it

---
 mlir/lib/Target/LLVMIR/DebugImporter.cpp     |  2 +-
 mlir/test/Target/LLVMIR/Import/debug-info.ll | 58 +++++++++++++++++++-
 2 files changed, 56 insertions(+), 4 deletions(-)

diff --git a/mlir/lib/Target/LLVMIR/DebugImporter.cpp b/mlir/lib/Target/LLVMIR/DebugImporter.cpp
index f4329dda2c4388..db499a02553fdf 100644
--- a/mlir/lib/Target/LLVMIR/DebugImporter.cpp
+++ b/mlir/lib/Target/LLVMIR/DebugImporter.cpp
@@ -468,7 +468,7 @@ DebugImporter::RecursionPruner::lookup(llvm::DINode *node) {
           // A replacement may contain additional unbound self-refs.
           return std::make_pair(replacement, mlir::WalkResult::advance());
         }
-        return std::make_pair(attr, mlir::WalkResult::skip());
+        return std::make_pair(attr, mlir::WalkResult::advance());
       });
 
   Attribute replacedAttr = replacer.replace(entry.attr);
diff --git a/mlir/test/Target/LLVMIR/Import/debug-info.ll b/mlir/test/Target/LLVMIR/Import/debug-info.ll
index 931f914c995bb6..ac1a6b79675c1d 100644
--- a/mlir/test/Target/LLVMIR/Import/debug-info.ll
+++ b/mlir/test/Target/LLVMIR/Import/debug-info.ll
@@ -610,7 +610,15 @@ declare !dbg !1 void @declaration()
 
 ; // -----
 
-; Ensure that repeated occurence of recursive subtree does not result in duplicate MLIR entries.
+; Ensure that repeated occurence of recursive subtree does not result in
+; duplicate MLIR entries.
+;
+; +--> B:B1 ----+
+; |     ^       v
+; A <---+------ B
+; |     v       ^
+; +--> B:B2 ----+
+; This should result in only one B instance.
 
 ; CHECK-DAG: #[[B1_INNER:.+]] = #llvm.di_derived_type<{{.*}}name = "B:B1", baseType = #[[B_SELF:.+]]>
 ; CHECK-DAG: #[[B2_INNER:.+]] = #llvm.di_derived_type<{{.*}}name = "B:B2", baseType = #[[B_SELF]]>
@@ -629,8 +637,6 @@ define void @class_field(ptr %arg1) !dbg !18 {
   ret void
 }
 
-declare void @llvm.dbg.value(metadata, metadata, metadata)
-
 !llvm.dbg.cu = !{!1}
 !llvm.module.flags = !{!0}
 !0 = !{i32 2, !"Debug Info Version", i32 3}
@@ -646,3 +652,49 @@ declare void @llvm.dbg.value(metadata, metadata, metadata)
 !9 = !{!7, !8}
 
 !18 = distinct !DISubprogram(name: "A", scope: !3, file: !2, spFlags: DISPFlagDefinition, unit: !1)
+
+; // -----
+
+; Ensure that recursive cycles with multiple entry points are cached correctly.
+;
+; +---- A ----+
+; v           v
+; B <-------> C
+; This should result in a cached instance of B --> C --> B_SELF to be reused
+; when visiting B from C (after visiting B from A).
+
+; CHECK-DAG: #[[C_INNER:.+]] = #llvm.di_composite_type<{{.*}}name = "C", {{.*}}scope = #[[A_SELF:[^ ]+]], {{.*}}elements = #[[C_INNER_TO_B_SELF:.+]]
+; CHECK-DAG: #[[C_INNER_TO_B_SELF:.+]] = #llvm.di_derived_type<{{.*}}name = "->B", {{.*}}baseType = #[[B_SELF:[^ ]+]]>
+; CHECK-DAG: #[[B_TO_C_INNER:.+]] = #llvm.di_derived_type<{{.*}}name = "->C", {{.*}}baseType = #[[C_INNER]]
+; CHECK-DAG: #[[B:.+]] = #llvm.di_composite_type<{{.*}}recId = [[B_RECID:.+]], {{.*}}name = "B", {{.*}}scope = #[[A_SELF]], {{.*}}elements = #[[B_TO_C_INNER]]
+; CHECK-DAG: #[[A_SELF]] = #llvm.di_composite_type<{{.*}}recId = [[A_RECID:.+]]>
+; CHECK-DAG: #[[B_SELF]] = #llvm.di_composite_type<{{.*}}recId = [[B_RECID]]>
+
+; CHECK-DAG: #[[TO_B:.+]] = #llvm.di_derived_type<{{.*}}name = "->B", {{.*}}baseType = #[[B]]
+; CHECK-DAG: #[[C_OUTER:.+]] = #llvm.di_composite_type<{{.*}}name = "C", {{.*}}scope = #[[A_SELF]], {{.*}}elements = #[[TO_B]]
+; CHECK-DAG: #[[TO_C:.+]] = #llvm.di_derived_type<{{.*}}name = "->C", {{.*}}baseType = #[[C_OUTER]]
+; CHECK-DAG: #[[A:.+]] = #llvm.di_composite_type<{{.*}}recId = [[A_RECID]], {{.*}}name = "A", {{.*}}elements = #[[TO_B]], #[[TO_C]]
+
+; CHECK-DAG: #llvm.di_subprogram<{{.*}}scope = #[[A]],
+
+define void @class_field(ptr %arg1) !dbg !18 {
+  ret void
+}
+
+!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 = !DICompositeType(tag: DW_TAG_class_type, name: "A", file: !2, line: 42, flags: DIFlagTypePassByReference | DIFlagNonTrivial, elements: !4)
+!5 = !DICompositeType(tag: DW_TAG_class_type, name: "B", scope: !3, file: !2, line: 42, flags: DIFlagTypePassByReference | DIFlagNonTrivial, elements: !10)
+!6 = !DICompositeType(tag: DW_TAG_class_type, name: "C", scope: !3, file: !2, line: 42, flags: DIFlagTypePassByReference | DIFlagNonTrivial, elements: !9)
+
+!7 = !DIDerivedType(tag: DW_TAG_member, name: "->B", file: !2, baseType: !5)
+!8 = !DIDerivedType(tag: DW_TAG_member, name: "->C", file: !2, baseType: !6)
+!4 = !{!7, !8}
+!9 = !{!7}
+!10 = !{!8}
+
+!18 = distinct !DISubprogram(name: "SP", scope: !3, file: !2, spFlags: DISPFlagDefinition, unit: !1)

>From a8e999cd8abb0fdbaf318f64ca77d304edf58244 Mon Sep 17 00:00:00 2001
From: Billy Zhu <billyzhu at modular.com>
Date: Tue, 2 Apr 2024 15:50:11 -0700
Subject: [PATCH 06/10] cleanup

---
 mlir/lib/Target/LLVMIR/DebugImporter.cpp | 12 ------------
 mlir/lib/Target/LLVMIR/DebugImporter.h   |  1 -
 2 files changed, 13 deletions(-)

diff --git a/mlir/lib/Target/LLVMIR/DebugImporter.cpp b/mlir/lib/Target/LLVMIR/DebugImporter.cpp
index db499a02553fdf..b5b709f00a07fd 100644
--- a/mlir/lib/Target/LLVMIR/DebugImporter.cpp
+++ b/mlir/lib/Target/LLVMIR/DebugImporter.cpp
@@ -29,8 +29,6 @@ DebugImporter::DebugImporter(ModuleOp mlirModule)
     : recursionPruner(mlirModule.getContext()),
       context(mlirModule.getContext()), mlirModule(mlirModule) {}
 
-DebugImporter::~DebugImporter() {}
-
 Location DebugImporter::translateFuncLocation(llvm::Function *func) {
   llvm::DISubprogram *subprogram = func->getSubprogram();
   if (!subprogram)
@@ -357,9 +355,6 @@ DINodeAttr DebugImporter::RecursionPruner::pruneOrPushTranslationStack(
   return nullptr;
 }
 
-/// Register the translated result of `node`. Returns the finalized result
-/// (with recId if recursive) and whether the result is self-contained
-/// (i.e. contains no unbound self-refs).
 std::pair<DINodeAttr, bool>
 DebugImporter::RecursionPruner::finalizeTranslation(llvm::DINode *node,
                                                     DINodeAttr result) {
@@ -411,8 +406,6 @@ DebugImporter::RecursionPruner::finalizeTranslation(llvm::DINode *node,
   return {result, state.unboundSelfRefs.empty()};
 }
 
-/// Pop off a frame from the translation stack after a node is done being
-/// translated.
 void DebugImporter::RecursionPruner::popTranslationStack(llvm::DINode *node) {
   // If `node` is not a potentially recursive type, it will not be on the
   // translation stack. Nothing to handle in this case.
@@ -444,11 +437,6 @@ void DebugImporter::RecursionPruner::popTranslationStack(llvm::DINode *node) {
   translationStack.pop_back();
 }
 
-/// Returns the cached result (if exists) with all known replacements applied.
-/// Also returns the set of unbound self-refs that are unresolved in this
-/// cached result.
-/// The cache entry will also be updated with the replaced result and with the
-/// applied replacements removed from the pendingReplacements map.
 std::pair<DINodeAttr, DenseSet<DIRecursiveTypeAttrInterface>>
 DebugImporter::RecursionPruner::lookup(llvm::DINode *node) {
   auto cacheIter = cache.find(node);
diff --git a/mlir/lib/Target/LLVMIR/DebugImporter.h b/mlir/lib/Target/LLVMIR/DebugImporter.h
index 7f21b836ac9f38..20f9b90154a358 100644
--- a/mlir/lib/Target/LLVMIR/DebugImporter.h
+++ b/mlir/lib/Target/LLVMIR/DebugImporter.h
@@ -30,7 +30,6 @@ namespace detail {
 class DebugImporter {
 public:
   DebugImporter(ModuleOp mlirModule);
-  ~DebugImporter();
 
   /// Translates the given LLVM debug location to an MLIR location.
   Location translateLoc(llvm::DILocation *loc);

>From 3a0842b1dd3f78c7c5db3720beb69b3e1f2576e6 Mon Sep 17 00:00:00 2001
From: Billy Zhu <billyzhu at modular.com>
Date: Wed, 3 Apr 2024 15:58:59 -0700
Subject: [PATCH 07/10] handle mutual recursion replacement

---
 mlir/lib/Target/LLVMIR/DebugImporter.cpp     | 83 +++++++++++++++++---
 mlir/test/Target/LLVMIR/Import/debug-info.ll | 53 +++++++++++++
 2 files changed, 124 insertions(+), 12 deletions(-)

diff --git a/mlir/lib/Target/LLVMIR/DebugImporter.cpp b/mlir/lib/Target/LLVMIR/DebugImporter.cpp
index b5b709f00a07fd..eaa546a8b8f834 100644
--- a/mlir/lib/Target/LLVMIR/DebugImporter.cpp
+++ b/mlir/lib/Target/LLVMIR/DebugImporter.cpp
@@ -318,6 +318,75 @@ getRecSelfConstructor(llvm::DINode *node) {
       .Default(CtorType());
 }
 
+/// An attribute replacer that replaces nested recursive decls with recursive
+/// self-references instead.
+///
+/// - Recurses down the attribute tree while replacing attributes based on the
+///   provided replacement map.
+/// - Keeps track of the currently open recursive declarations, and upon
+///   encountering a duplicate declaration, replace with a self-ref instead.
+static Attribute replaceAndPruneRecursiveTypesImpl(
+    Attribute node,
+    const DenseMap<DIRecursiveTypeAttrInterface, DINodeAttr> &mapping,
+    DenseSet<DistinctAttr> &openDecls) {
+  DistinctAttr recId;
+  if (auto recType = dyn_cast<DIRecursiveTypeAttrInterface>(node)) {
+    recId = recType.getRecId();
+
+    // Configure context.
+    if (recId) {
+      if (recType.isRecSelf()) {
+        // Replace selfRef based on the provided mapping.
+        if (DINodeAttr replacement = mapping.lookup(recType))
+          return replaceAndPruneRecursiveTypesImpl(replacement, mapping,
+                                                   openDecls);
+        return node;
+      }
+
+      auto [_, inserted] = openDecls.insert(recId);
+      if (!inserted) {
+        // This is a nested decl. Replace with recSelf.
+        return recType.getRecSelf(recId);
+      }
+    }
+  }
+
+  // Collect sub attrs.
+  SmallVector<Attribute> attrs;
+  SmallVector<Type> types;
+  node.walkImmediateSubElements(
+      [&attrs](Attribute attr) { attrs.push_back(attr); },
+      [&types](Type type) { types.push_back(type); });
+
+  // Recurse into attributes.
+  bool changed = false;
+  for (auto it = attrs.begin(); it != attrs.end(); it++) {
+    Attribute replaced =
+        replaceAndPruneRecursiveTypesImpl(*it, mapping, openDecls);
+    if (replaced != *it) {
+      *it = replaced;
+      changed = true;
+    }
+  }
+
+  Attribute result = node;
+  if (changed)
+    result = result.replaceImmediateSubElements(attrs, types);
+
+  // Reset context.
+  if (recId)
+    openDecls.erase(recId);
+
+  return result;
+}
+
+static Attribute replaceAndPruneRecursiveTypes(
+    DINodeAttr node,
+    const DenseMap<DIRecursiveTypeAttrInterface, DINodeAttr> &mapping) {
+  DenseSet<DistinctAttr> openDecls;
+  return replaceAndPruneRecursiveTypesImpl(node, mapping, openDecls);
+}
+
 DINodeAttr DebugImporter::RecursionPruner::pruneOrPushTranslationStack(
     llvm::DINode *node) {
   // Lookup the cache first.
@@ -448,18 +517,8 @@ DebugImporter::RecursionPruner::lookup(llvm::DINode *node) {
   if (entry.pendingReplacements.empty())
     return std::make_pair(entry.attr, DenseSet<DIRecursiveTypeAttrInterface>{});
 
-  AttrTypeReplacer replacer;
-  replacer.addReplacement(
-      [&entry](DIRecursiveTypeAttrInterface attr)
-          -> std::optional<std::pair<Attribute, WalkResult>> {
-        if (auto replacement = entry.pendingReplacements.lookup(attr)) {
-          // A replacement may contain additional unbound self-refs.
-          return std::make_pair(replacement, mlir::WalkResult::advance());
-        }
-        return std::make_pair(attr, mlir::WalkResult::advance());
-      });
-
-  Attribute replacedAttr = replacer.replace(entry.attr);
+  Attribute replacedAttr =
+      replaceAndPruneRecursiveTypes(entry.attr, entry.pendingReplacements);
   DINodeAttr result = cast<DINodeAttr>(replacedAttr);
 
   // Update cache entry to save replaced version and remove already-applied
diff --git a/mlir/test/Target/LLVMIR/Import/debug-info.ll b/mlir/test/Target/LLVMIR/Import/debug-info.ll
index ac1a6b79675c1d..2218e6cd5b7bbd 100644
--- a/mlir/test/Target/LLVMIR/Import/debug-info.ll
+++ b/mlir/test/Target/LLVMIR/Import/debug-info.ll
@@ -698,3 +698,56 @@ define void @class_field(ptr %arg1) !dbg !18 {
 !10 = !{!8}
 
 !18 = distinct !DISubprogram(name: "SP", scope: !3, file: !2, spFlags: DISPFlagDefinition, unit: !1)
+
+; // -----
+
+; Ensures that replacing a nested mutually recursive decl does not result in
+; nested duplicate recursive decls.
+;
+; A ---> B <--> C
+; ^             ^
+; +-------------+
+
+; CHECK-DAG: #[[A:.+]] = #llvm.di_composite_type<{{.*}}recId = [[A_RECID:.+]], {{.*}}name = "A", {{.*}}elements = #[[A_TO_B:.+]], #[[A_TO_C:.+]]>
+; CHECK-DAG: #llvm.di_subprogram<{{.*}}scope = #[[A]],
+; CHECK-DAG: #[[A_TO_B]] = #llvm.di_derived_type<{{.*}}name = "->B", {{.*}}baseType = #[[B_FROM_A:.+]]>
+; CHECK-DAG: #[[A_TO_C]] = #llvm.di_derived_type<{{.*}}name = "->C", {{.*}}baseType = #[[C_FROM_A:.+]]>
+
+; CHECK-DAG: #[[B_FROM_A]] = #llvm.di_composite_type<{{.*}}recId = [[B_RECID:.+]], {{.*}}name = "B", {{.*}}elements = #[[B_TO_C:.+]]>
+; CHECK-DAG: #[[B_TO_C]] = #llvm.di_derived_type<{{.*}}name = "->C", {{.*}}baseType = #[[C_FROM_B:.+]]>
+; CHECK-DAG: #[[C_FROM_B]] = #llvm.di_composite_type<{{.*}}recId = [[C_RECID:.+]], {{.*}}name = "C", {{.*}}elements = #[[TO_A_SELF:.+]], #[[TO_B_SELF:.+]], #[[TO_C_SELF:.+]]>
+
+; CHECK-DAG: #[[C_FROM_A]] = #llvm.di_composite_type<{{.*}}recId = [[C_RECID]], {{.*}}name = "C", {{.*}}elements = #[[TO_A_SELF]], #[[TO_B_INNER:.+]], #[[TO_C_SELF]]
+; CHECK-DAG: #[[TO_B_INNER]] = #llvm.di_derived_type<{{.*}}name = "->B", {{.*}}baseType = #[[B_INNER:.+]]>
+; CHECK-DAG: #[[B_INNER]] = #llvm.di_composite_type<{{.*}}name = "B", {{.*}}elements = #[[TO_C_SELF]]>
+
+; CHECK-DAG: #[[TO_A_SELF]] = #llvm.di_derived_type<{{.*}}name = "->A", {{.*}}baseType = #[[A_SELF:.+]]>
+; CHECK-DAG: #[[TO_B_SELF]] = #llvm.di_derived_type<{{.*}}name = "->B", {{.*}}baseType = #[[B_SELF:.+]]>
+; CHECK-DAG: #[[TO_C_SELF]] = #llvm.di_derived_type<{{.*}}name = "->C", {{.*}}baseType = #[[C_SELF:.+]]>
+; CHECK-DAG: #[[A_SELF]] = #llvm.di_composite_type<{{.*}}recId = [[A_RECID]]>
+; CHECK-DAG: #[[B_SELF]] = #llvm.di_composite_type<{{.*}}recId = [[B_RECID]]>
+; CHECK-DAG: #[[C_SELF]] = #llvm.di_composite_type<{{.*}}recId = [[C_RECID]]>
+
+define void @class_field(ptr %arg1) !dbg !18 {
+  ret void
+}
+
+!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 = !DICompositeType(tag: DW_TAG_class_type, name: "A", file: !2, line: 42, flags: DIFlagTypePassByReference | DIFlagNonTrivial, elements: !9)
+!4 = !DICompositeType(tag: DW_TAG_class_type, name: "B", file: !2, line: 42, flags: DIFlagTypePassByReference | DIFlagNonTrivial, elements: !10)
+!5 = !DICompositeType(tag: DW_TAG_class_type, name: "C", file: !2, line: 42, flags: DIFlagTypePassByReference | DIFlagNonTrivial, elements: !11)
+
+!6 = !DIDerivedType(tag: DW_TAG_member, name: "->A", file: !2, baseType: !3)
+!7 = !DIDerivedType(tag: DW_TAG_member, name: "->B", file: !2, baseType: !4)
+!8 = !DIDerivedType(tag: DW_TAG_member, name: "->C", file: !2, baseType: !5)
+
+!9 = !{!7, !8} ; A -> B, C
+!10 = !{!8} ; B -> C
+!11 = !{!6, !7, !8} ; C -> A, B, C
+
+!18 = distinct !DISubprogram(name: "SP", scope: !3, file: !2, spFlags: DISPFlagDefinition, unit: !1)

>From 6f57bc1c1697cb10cb2f61485c476a0ede73bf77 Mon Sep 17 00:00:00 2001
From: Billy Zhu <billyzhu at modular.com>
Date: Wed, 3 Apr 2024 16:50:07 -0700
Subject: [PATCH 08/10] replace with iterative version instead

---
 mlir/lib/Target/LLVMIR/DebugImporter.cpp | 140 ++++++++++++++---------
 1 file changed, 84 insertions(+), 56 deletions(-)

diff --git a/mlir/lib/Target/LLVMIR/DebugImporter.cpp b/mlir/lib/Target/LLVMIR/DebugImporter.cpp
index eaa546a8b8f834..ecc179eb68136c 100644
--- a/mlir/lib/Target/LLVMIR/DebugImporter.cpp
+++ b/mlir/lib/Target/LLVMIR/DebugImporter.cpp
@@ -318,73 +318,101 @@ getRecSelfConstructor(llvm::DINode *node) {
       .Default(CtorType());
 }
 
-/// An attribute replacer that replaces nested recursive decls with recursive
-/// self-references instead.
-///
+/// An iterative attribute replacer that also handles pruning recursive types.
 /// - Recurses down the attribute tree while replacing attributes based on the
 ///   provided replacement map.
 /// - Keeps track of the currently open recursive declarations, and upon
-///   encountering a duplicate declaration, replace with a self-ref instead.
-static Attribute replaceAndPruneRecursiveTypesImpl(
-    Attribute node,
-    const DenseMap<DIRecursiveTypeAttrInterface, DINodeAttr> &mapping,
-    DenseSet<DistinctAttr> &openDecls) {
-  DistinctAttr recId;
-  if (auto recType = dyn_cast<DIRecursiveTypeAttrInterface>(node)) {
-    recId = recType.getRecId();
-
-    // Configure context.
-    if (recId) {
-      if (recType.isRecSelf()) {
-        // Replace selfRef based on the provided mapping.
-        if (DINodeAttr replacement = mapping.lookup(recType))
-          return replaceAndPruneRecursiveTypesImpl(replacement, mapping,
-                                                   openDecls);
-        return node;
-      }
+///   encountering a duplicate declaration, replace with a self-reference.
+static Attribute replaceAndPruneRecursiveTypes(
+    Attribute baseNode,
+    const DenseMap<DIRecursiveTypeAttrInterface, DINodeAttr> &mapping) {
 
-      auto [_, inserted] = openDecls.insert(recId);
-      if (!inserted) {
-        // This is a nested decl. Replace with recSelf.
-        return recType.getRecSelf(recId);
+  struct ReplacementState {
+    // The attribute being replaced.
+    Attribute node;
+    // The nested elements of `node`.
+    SmallVector<Attribute> attrs;
+    SmallVector<Type> types;
+    // The current attr being walked on (index into `attrs`).
+    size_t attrIndex;
+    // Whether or not any attr was replaced.
+    bool changed;
+
+    void replaceCurrAttr(Attribute attr) {
+      Attribute &target = attrs[attrIndex];
+      if (attr != target) {
+        changed = true;
+        target = attr;
       }
     }
-  }
+  };
 
-  // Collect sub attrs.
-  SmallVector<Attribute> attrs;
-  SmallVector<Type> types;
-  node.walkImmediateSubElements(
-      [&attrs](Attribute attr) { attrs.push_back(attr); },
-      [&types](Type type) { types.push_back(type); });
-
-  // Recurse into attributes.
-  bool changed = false;
-  for (auto it = attrs.begin(); it != attrs.end(); it++) {
-    Attribute replaced =
-        replaceAndPruneRecursiveTypesImpl(*it, mapping, openDecls);
-    if (replaced != *it) {
-      *it = replaced;
-      changed = true;
-    }
-  }
+  // Every iteration, perform replacement on the attribute at `attrIndex` at the
+  // top of `workStack`.
+  // If `attrIndex` reaches past the size of `attrs`, replace `node` with
+  // `attrs` & `types`, and pop off a stack frame and assign the replacement to
+  // the attr being replaced on the previous stack frame.
+  SmallVector<ReplacementState> workStack;
+  workStack.push_back({nullptr, {baseNode}, {}, 0, false});
 
-  Attribute result = node;
-  if (changed)
-    result = result.replaceImmediateSubElements(attrs, types);
+  DenseSet<DistinctAttr> openDecls;
+  while (workStack.size() > 1 || workStack.back().attrIndex == 0) {
+    ReplacementState &state = workStack.back();
+
+    // Check for popping condition.
+    if (state.attrIndex == state.attrs.size()) {
+      Attribute result = state.node;
+      if (state.changed)
+        result = result.replaceImmediateSubElements(state.attrs, state.types);
+
+      // Reset context.
+      if (auto recType = dyn_cast<DIRecursiveTypeAttrInterface>(state.node))
+        if (DistinctAttr recId = recType.getRecId())
+          openDecls.erase(recId);
+
+      workStack.pop_back();
+      ReplacementState &prevState = workStack.back();
+      prevState.replaceCurrAttr(result);
+      ++prevState.attrIndex;
+      continue;
+    }
 
-  // Reset context.
-  if (recId)
-    openDecls.erase(recId);
+    Attribute node = state.attrs[state.attrIndex];
+    if (auto recType = dyn_cast<DIRecursiveTypeAttrInterface>(node)) {
+      if (DistinctAttr recId = recType.getRecId()) {
+        if (recType.isRecSelf()) {
+          // Replace selfRef based on the provided mapping and re-walk from the
+          // replacement node (do not increment attrIndex).
+          if (DINodeAttr replacement = mapping.lookup(recType)) {
+            state.replaceCurrAttr(replacement);
+            continue;
+          }
+
+          // Otherwise, nothing to do. Advance to next attr.
+          ++state.attrIndex;
+          continue;
+        }
+
+        // Configure context.
+        auto [_, inserted] = openDecls.insert(recId);
+        if (!inserted) {
+          // This is a nested decl. Replace with recSelf. Nothing more to do.
+          state.replaceCurrAttr(recType.getRecSelf(recId));
+          ++state.attrIndex;
+          continue;
+        }
+      }
+    }
 
-  return result;
-}
+    // Recurse into this node.
+    workStack.push_back({node, {}, {}, 0, false});
+    ReplacementState &newState = workStack.back();
+    node.walkImmediateSubElements(
+        [&newState](Attribute attr) { newState.attrs.push_back(attr); },
+        [&newState](Type type) { newState.types.push_back(type); });
+  };
 
-static Attribute replaceAndPruneRecursiveTypes(
-    DINodeAttr node,
-    const DenseMap<DIRecursiveTypeAttrInterface, DINodeAttr> &mapping) {
-  DenseSet<DistinctAttr> openDecls;
-  return replaceAndPruneRecursiveTypesImpl(node, mapping, openDecls);
+  return workStack.back().attrs.front();
 }
 
 DINodeAttr DebugImporter::RecursionPruner::pruneOrPushTranslationStack(

>From 628f5201a47facc433e5195cacaa69e9b861a67c Mon Sep 17 00:00:00 2001
From: Billy Zhu <billyzhu at modular.com>
Date: Thu, 4 Apr 2024 11:12:58 -0700
Subject: [PATCH 09/10] add cache to replacer

---
 mlir/lib/Target/LLVMIR/DebugImporter.cpp | 25 ++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/mlir/lib/Target/LLVMIR/DebugImporter.cpp b/mlir/lib/Target/LLVMIR/DebugImporter.cpp
index ecc179eb68136c..2113af1aa44299 100644
--- a/mlir/lib/Target/LLVMIR/DebugImporter.cpp
+++ b/mlir/lib/Target/LLVMIR/DebugImporter.cpp
@@ -13,6 +13,7 @@
 #include "mlir/IR/Location.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/ScopeExit.h"
+#include "llvm/ADT/SetOperations.h"
 #include "llvm/ADT/TypeSwitch.h"
 #include "llvm/BinaryFormat/Dwarf.h"
 #include "llvm/IR/Constants.h"
@@ -355,6 +356,14 @@ static Attribute replaceAndPruneRecursiveTypes(
   SmallVector<ReplacementState> workStack;
   workStack.push_back({nullptr, {baseNode}, {}, 0, false});
 
+  // Replacement cache that remembers the context in which the cache is valid.
+  // All unboundRecIds must be in openDecls at time of lookup.
+  struct CacheWithContext {
+    DenseSet<DistinctAttr> unboundRecIds;
+    Attribute entry;
+  };
+  DenseMap<Attribute, CacheWithContext> replacementCache;
+
   DenseSet<DistinctAttr> openDecls;
   while (workStack.size() > 1 || workStack.back().attrIndex == 0) {
     ReplacementState &state = workStack.back();
@@ -370,6 +379,8 @@ static Attribute replaceAndPruneRecursiveTypes(
         if (DistinctAttr recId = recType.getRecId())
           openDecls.erase(recId);
 
+      replacementCache[state.node] = CacheWithContext{openDecls, result};
+
       workStack.pop_back();
       ReplacementState &prevState = workStack.back();
       prevState.replaceCurrAttr(result);
@@ -378,6 +389,20 @@ static Attribute replaceAndPruneRecursiveTypes(
     }
 
     Attribute node = state.attrs[state.attrIndex];
+
+    // Lookup in cache first.
+    if (auto it = replacementCache.find(node); it != replacementCache.end()) {
+      // If all the requried recIds are open decls, use cache.
+      if (llvm::set_is_subset(it->second.unboundRecIds, openDecls)) {
+        state.replaceCurrAttr(it->second.entry);
+        ++state.attrIndex;
+        continue;
+      }
+
+      // Otherwise, the cache entry is stale and can be removed now.
+      replacementCache.erase(it);
+    }
+
     if (auto recType = dyn_cast<DIRecursiveTypeAttrInterface>(node)) {
       if (DistinctAttr recId = recType.getRecId()) {
         if (recType.isRecSelf()) {

>From 9e2cb3a8c948f2d881e9ec91700e09d94eb3f86d Mon Sep 17 00:00:00 2001
From: Billy Zhu <billyzhu at modular.com>
Date: Thu, 4 Apr 2024 13:31:44 -0700
Subject: [PATCH 10/10] simplify away cache with holes

---
 mlir/lib/Target/LLVMIR/DebugImporter.cpp     | 223 +++----------------
 mlir/lib/Target/LLVMIR/DebugImporter.h       |  49 ++--
 mlir/test/Target/LLVMIR/Import/debug-info.ll |  25 +--
 3 files changed, 59 insertions(+), 238 deletions(-)

diff --git a/mlir/lib/Target/LLVMIR/DebugImporter.cpp b/mlir/lib/Target/LLVMIR/DebugImporter.cpp
index 2113af1aa44299..9719b614f60584 100644
--- a/mlir/lib/Target/LLVMIR/DebugImporter.cpp
+++ b/mlir/lib/Target/LLVMIR/DebugImporter.cpp
@@ -319,139 +319,8 @@ getRecSelfConstructor(llvm::DINode *node) {
       .Default(CtorType());
 }
 
-/// An iterative attribute replacer that also handles pruning recursive types.
-/// - Recurses down the attribute tree while replacing attributes based on the
-///   provided replacement map.
-/// - Keeps track of the currently open recursive declarations, and upon
-///   encountering a duplicate declaration, replace with a self-reference.
-static Attribute replaceAndPruneRecursiveTypes(
-    Attribute baseNode,
-    const DenseMap<DIRecursiveTypeAttrInterface, DINodeAttr> &mapping) {
-
-  struct ReplacementState {
-    // The attribute being replaced.
-    Attribute node;
-    // The nested elements of `node`.
-    SmallVector<Attribute> attrs;
-    SmallVector<Type> types;
-    // The current attr being walked on (index into `attrs`).
-    size_t attrIndex;
-    // Whether or not any attr was replaced.
-    bool changed;
-
-    void replaceCurrAttr(Attribute attr) {
-      Attribute &target = attrs[attrIndex];
-      if (attr != target) {
-        changed = true;
-        target = attr;
-      }
-    }
-  };
-
-  // Every iteration, perform replacement on the attribute at `attrIndex` at the
-  // top of `workStack`.
-  // If `attrIndex` reaches past the size of `attrs`, replace `node` with
-  // `attrs` & `types`, and pop off a stack frame and assign the replacement to
-  // the attr being replaced on the previous stack frame.
-  SmallVector<ReplacementState> workStack;
-  workStack.push_back({nullptr, {baseNode}, {}, 0, false});
-
-  // Replacement cache that remembers the context in which the cache is valid.
-  // All unboundRecIds must be in openDecls at time of lookup.
-  struct CacheWithContext {
-    DenseSet<DistinctAttr> unboundRecIds;
-    Attribute entry;
-  };
-  DenseMap<Attribute, CacheWithContext> replacementCache;
-
-  DenseSet<DistinctAttr> openDecls;
-  while (workStack.size() > 1 || workStack.back().attrIndex == 0) {
-    ReplacementState &state = workStack.back();
-
-    // Check for popping condition.
-    if (state.attrIndex == state.attrs.size()) {
-      Attribute result = state.node;
-      if (state.changed)
-        result = result.replaceImmediateSubElements(state.attrs, state.types);
-
-      // Reset context.
-      if (auto recType = dyn_cast<DIRecursiveTypeAttrInterface>(state.node))
-        if (DistinctAttr recId = recType.getRecId())
-          openDecls.erase(recId);
-
-      replacementCache[state.node] = CacheWithContext{openDecls, result};
-
-      workStack.pop_back();
-      ReplacementState &prevState = workStack.back();
-      prevState.replaceCurrAttr(result);
-      ++prevState.attrIndex;
-      continue;
-    }
-
-    Attribute node = state.attrs[state.attrIndex];
-
-    // Lookup in cache first.
-    if (auto it = replacementCache.find(node); it != replacementCache.end()) {
-      // If all the requried recIds are open decls, use cache.
-      if (llvm::set_is_subset(it->second.unboundRecIds, openDecls)) {
-        state.replaceCurrAttr(it->second.entry);
-        ++state.attrIndex;
-        continue;
-      }
-
-      // Otherwise, the cache entry is stale and can be removed now.
-      replacementCache.erase(it);
-    }
-
-    if (auto recType = dyn_cast<DIRecursiveTypeAttrInterface>(node)) {
-      if (DistinctAttr recId = recType.getRecId()) {
-        if (recType.isRecSelf()) {
-          // Replace selfRef based on the provided mapping and re-walk from the
-          // replacement node (do not increment attrIndex).
-          if (DINodeAttr replacement = mapping.lookup(recType)) {
-            state.replaceCurrAttr(replacement);
-            continue;
-          }
-
-          // Otherwise, nothing to do. Advance to next attr.
-          ++state.attrIndex;
-          continue;
-        }
-
-        // Configure context.
-        auto [_, inserted] = openDecls.insert(recId);
-        if (!inserted) {
-          // This is a nested decl. Replace with recSelf. Nothing more to do.
-          state.replaceCurrAttr(recType.getRecSelf(recId));
-          ++state.attrIndex;
-          continue;
-        }
-      }
-    }
-
-    // Recurse into this node.
-    workStack.push_back({node, {}, {}, 0, false});
-    ReplacementState &newState = workStack.back();
-    node.walkImmediateSubElements(
-        [&newState](Attribute attr) { newState.attrs.push_back(attr); },
-        [&newState](Type type) { newState.types.push_back(type); });
-  };
-
-  return workStack.back().attrs.front();
-}
-
 DINodeAttr DebugImporter::RecursionPruner::pruneOrPushTranslationStack(
     llvm::DINode *node) {
-  // Lookup the cache first.
-  auto [result, unboundSelfRefs] = lookup(node);
-  if (result) {
-    // Need to inject unbound self-refs into the previous layer.
-    if (!unboundSelfRefs.empty())
-      translationStack.back().second.unboundSelfRefs.insert(
-          unboundSelfRefs.begin(), unboundSelfRefs.end());
-    return result;
-  }
-
   // If the node type is capable of being recursive, check if it's seen
   // before.
   auto recSelfCtor = getRecSelfConstructor(node);
@@ -465,7 +334,11 @@ DINodeAttr DebugImporter::RecursionPruner::pruneOrPushTranslationStack(
       // a different self-reference. Use that if possible.
       DIRecursiveTypeAttrInterface recSelf = iter->second.recSelf;
       if (!recSelf) {
-        DistinctAttr recId = DistinctAttr::create(UnitAttr::get(context));
+        DistinctAttr recId = nodeToRecId.lookup(node);
+        if (!recId) {
+          recId = DistinctAttr::create(UnitAttr::get(context));
+          nodeToRecId[node] = recId;
+        }
         recSelf = recSelfCtor(recId);
         iter->second.recSelf = recSelf;
       }
@@ -474,7 +347,8 @@ DINodeAttr DebugImporter::RecursionPruner::pruneOrPushTranslationStack(
       return cast<DINodeAttr>(recSelf);
     }
   }
-  return nullptr;
+
+  return lookup(node);
 }
 
 std::pair<DINodeAttr, bool>
@@ -493,39 +367,18 @@ DebugImporter::RecursionPruner::finalizeTranslation(llvm::DINode *node,
   if (DIRecursiveTypeAttrInterface recSelf = state.recSelf) {
     auto recType = cast<DIRecursiveTypeAttrInterface>(result);
     result = cast<DINodeAttr>(recType.withRecId(recSelf.getRecId()));
-
     // Remove this recSelf from the set of unbound selfRefs.
     state.unboundSelfRefs.erase(recSelf);
-
-    // Insert the newly resolved recursive type into the cache entries that
-    // rely on it.
-    // Only need to look at the caches at this level.
-    uint64_t numRemaining = state.cacheSize;
-    for (CachedTranslation &cacheEntry :
-         llvm::make_second_range(llvm::reverse(cache))) {
-      if (numRemaining == 0)
-        break;
-      --numRemaining;
-
-      if (auto refIter = cacheEntry.pendingReplacements.find(recSelf);
-          refIter != cacheEntry.pendingReplacements.end())
-        refIter->second = result;
-    }
   }
 
-  // Insert the current result into the cache.
-  state.cacheSize++;
-  auto [iter, inserted] = cache.try_emplace(node);
-  assert(inserted && "invalid state: caching the same DINode twice");
-  iter->second.attr = result;
-
-  // If this node had any unbound self-refs free when it is registered into
-  // the cache, set up replacement placeholders: This result will need these
-  // unbound self-refs to be replaced before being used.
-  for (DIRecursiveTypeAttrInterface selfRef : state.unboundSelfRefs)
-    iter->second.pendingReplacements.try_emplace(selfRef, nullptr);
-
-  return {result, state.unboundSelfRefs.empty()};
+  // Insert the result into our internal cache if it's not self-contained.
+  if (!state.unboundSelfRefs.empty()) {
+    auto [_, inserted] = dependentCache.try_emplace(
+        node, DependentTranslation{result, state.unboundSelfRefs});
+    assert(inserted && "invalid state: caching the same DINode twice");
+    return {result, false};
+  }
+  return {result, true};
 }
 
 void DebugImporter::RecursionPruner::popTranslationStack(llvm::DINode *node) {
@@ -540,10 +393,7 @@ void DebugImporter::RecursionPruner::popTranslationStack(llvm::DINode *node) {
   if (translationStack.size() == 1) {
     assert(currLayerState.unboundSelfRefs.empty() &&
            "internal error: unbound recursive self reference at top level.");
-    assert(currLayerState.cacheSize == cache.size() &&
-           "internal error: inconsistent cache size");
     translationStack.pop_back();
-    cache.clear();
     return;
   }
 
@@ -551,45 +401,22 @@ void DebugImporter::RecursionPruner::popTranslationStack(llvm::DINode *node) {
   TranslationState &nextLayerState = (++translationStack.rbegin())->second;
   nextLayerState.unboundSelfRefs.insert(currLayerState.unboundSelfRefs.begin(),
                                         currLayerState.unboundSelfRefs.end());
-
-  // The current layer cache is now considered part of the lower layer cache.
-  nextLayerState.cacheSize += currLayerState.cacheSize;
-
-  // Finally pop off this layer when all bookkeeping is done.
   translationStack.pop_back();
 }
 
-std::pair<DINodeAttr, DenseSet<DIRecursiveTypeAttrInterface>>
-DebugImporter::RecursionPruner::lookup(llvm::DINode *node) {
-  auto cacheIter = cache.find(node);
-  if (cacheIter == cache.end())
+DINodeAttr DebugImporter::RecursionPruner::lookup(llvm::DINode *node) {
+  auto cacheIter = dependentCache.find(node);
+  if (cacheIter == dependentCache.end())
     return {};
 
-  CachedTranslation &entry = cacheIter->second;
-
-  if (entry.pendingReplacements.empty())
-    return std::make_pair(entry.attr, DenseSet<DIRecursiveTypeAttrInterface>{});
-
-  Attribute replacedAttr =
-      replaceAndPruneRecursiveTypes(entry.attr, entry.pendingReplacements);
-  DINodeAttr result = cast<DINodeAttr>(replacedAttr);
-
-  // Update cache entry to save replaced version and remove already-applied
-  // replacements.
-  entry.attr = result;
-  DenseSet<DIRecursiveTypeAttrInterface> unboundRefs;
-  DenseSet<DIRecursiveTypeAttrInterface> boundRefs;
-  for (auto [refSelf, replacement] : entry.pendingReplacements) {
-    if (replacement)
-      boundRefs.insert(refSelf);
-    else
-      unboundRefs.insert(refSelf);
-  }
-
-  for (DIRecursiveTypeAttrInterface ref : boundRefs)
-    entry.pendingReplacements.erase(ref);
+  DependentTranslation &entry = cacheIter->second;
+  if (llvm::set_is_subset(entry.unboundSelfRefs,
+                          translationStack.back().second.unboundSelfRefs))
+    return entry.attr;
 
-  return std::make_pair(result, unboundRefs);
+  // Stale cache entry.
+  dependentCache.erase(cacheIter);
+  return {};
 }
 
 //===----------------------------------------------------------------------===//
diff --git a/mlir/lib/Target/LLVMIR/DebugImporter.h b/mlir/lib/Target/LLVMIR/DebugImporter.h
index 20f9b90154a358..8b22dc63456775 100644
--- a/mlir/lib/Target/LLVMIR/DebugImporter.h
+++ b/mlir/lib/Target/LLVMIR/DebugImporter.h
@@ -116,12 +116,11 @@ class DebugImporter {
   public:
     RecursionPruner(MLIRContext *context) : context(context) {}
 
-    /// If this node was previously cached, returns the cached result.
     /// If this node is a recursive instance that was previously seen, returns a
-    /// self-reference.
-    /// Otherwise, returns null attr, and a translation stack frame is created
-    /// for this node. Expects `finalizeTranslation` & `popTranslationStack`
-    /// to be called on this node later.
+    /// self-reference. If this node was previously cached, returns the cached
+    /// result. Otherwise, returns null attr, and a translation stack frame is
+    /// created for this node. Expects `finalizeTranslation` &
+    /// `popTranslationStack` to be called on this node later.
     DINodeAttr pruneOrPushTranslationStack(llvm::DINode *node);
 
     /// Register the translated result of `node`. Returns the finalized result
@@ -135,33 +134,27 @@ class DebugImporter {
     void popTranslationStack(llvm::DINode *node);
 
   private:
-    /// Returns the cached result (if exists) with all known replacements
-    /// applied. Also returns the set of unbound self-refs that are unresolved
-    /// in this cached result. The cache entry will also be updated with the
-    /// replaced result and with the applied replacements removed from the
-    /// pendingReplacements map.
-    std::pair<DINodeAttr, DenseSet<DIRecursiveTypeAttrInterface>>
-    lookup(llvm::DINode *node);
+    /// Returns the cached result (if exists) or null.
+    /// The cache entry will be removed if not all of its dependent self-refs
+    /// exists.
+    DINodeAttr lookup(llvm::DINode *node);
 
     MLIRContext *context;
 
-    /// A lazy cached translation that contains the translated attribute as well
-    /// as any unbound self-references that need to be replaced at lookup.
-    struct CachedTranslation {
+    /// A cached translation that contains the translated attribute as well
+    /// as any unbound self-references that it depends on.
+    struct DependentTranslation {
       /// The translated attr. May contain unbound self-references for other
       /// recursive attrs.
       DINodeAttr attr;
-      /// The pending replacements that need to be run on this `attr` before it
-      /// can be used.
-      /// Each key is a recursive self-ref, and each value is a recursive decl
-      /// that may contain additional unbound self-refs that need to be
-      /// replaced. Each replacement will be applied at most once. Once a
-      /// replacement is applied, the cached `attr` will be updated, and the
-      /// replacement will be removed from this map.
-      DenseMap<DIRecursiveTypeAttrInterface, DINodeAttr> pendingReplacements;
+      /// The set of unbound self-refs that this cached entry refers to. All
+      /// these self-refs must exist for the cached entry to be valid.
+      DenseSet<DIRecursiveTypeAttrInterface> unboundSelfRefs;
     };
     /// A mapping between LLVM debug metadata and the corresponding attribute.
-    llvm::MapVector<llvm::DINode *, CachedTranslation> cache;
+    /// Only contains those with unboundSelfRefs. Fully self-contained attrs
+    /// will be cached by the outer main translator.
+    DenseMap<llvm::DINode *, DependentTranslation> dependentCache;
 
     /// Each potentially recursive node will have a TranslationState pushed onto
     /// the `translationStack` to keep track of whether this node is actually
@@ -171,9 +164,6 @@ class DebugImporter {
       /// instance of itself is seen while translating it). Null if this node
       /// has not been seen again deeper in the translation stack.
       DIRecursiveTypeAttrInterface recSelf;
-      /// The number of cache entries belonging to this layer of the translation
-      /// stack. This corresponds to the last `cacheSize` entries of `cache`.
-      uint64_t cacheSize = 0;
       /// All the unbound recursive self references in this layer of the
       /// translation stack.
       DenseSet<DIRecursiveTypeAttrInterface> unboundSelfRefs;
@@ -184,6 +174,11 @@ class DebugImporter {
     /// later when the stack is deeper, the node is recursive, and its
     /// TranslationState is assigned a recSelf.
     llvm::MapVector<llvm::DINode *, TranslationState> translationStack;
+
+    /// A mapping between DINodes that are recursive, and their assigned recId.
+    /// This is kept so that repeated occurrences of the same node can reuse the
+    /// same ID and be deduplicated.
+    DenseMap<llvm::DINode *, DistinctAttr> nodeToRecId;
   };
   RecursionPruner recursionPruner;
 
diff --git a/mlir/test/Target/LLVMIR/Import/debug-info.ll b/mlir/test/Target/LLVMIR/Import/debug-info.ll
index 2218e6cd5b7bbd..e2ef94617dd7d3 100644
--- a/mlir/test/Target/LLVMIR/Import/debug-info.ll
+++ b/mlir/test/Target/LLVMIR/Import/debug-info.ll
@@ -663,19 +663,18 @@ define void @class_field(ptr %arg1) !dbg !18 {
 ; This should result in a cached instance of B --> C --> B_SELF to be reused
 ; when visiting B from C (after visiting B from A).
 
-; CHECK-DAG: #[[C_INNER:.+]] = #llvm.di_composite_type<{{.*}}name = "C", {{.*}}scope = #[[A_SELF:[^ ]+]], {{.*}}elements = #[[C_INNER_TO_B_SELF:.+]]
-; CHECK-DAG: #[[C_INNER_TO_B_SELF:.+]] = #llvm.di_derived_type<{{.*}}name = "->B", {{.*}}baseType = #[[B_SELF:[^ ]+]]>
-; CHECK-DAG: #[[B_TO_C_INNER:.+]] = #llvm.di_derived_type<{{.*}}name = "->C", {{.*}}baseType = #[[C_INNER]]
-; CHECK-DAG: #[[B:.+]] = #llvm.di_composite_type<{{.*}}recId = [[B_RECID:.+]], {{.*}}name = "B", {{.*}}scope = #[[A_SELF]], {{.*}}elements = #[[B_TO_C_INNER]]
-; CHECK-DAG: #[[A_SELF]] = #llvm.di_composite_type<{{.*}}recId = [[A_RECID:.+]]>
-; CHECK-DAG: #[[B_SELF]] = #llvm.di_composite_type<{{.*}}recId = [[B_RECID]]>
+; CHECK-DAG: #[[A:.+]] = #llvm.di_composite_type<{{.*}}name = "A", {{.*}}elements = #[[TO_B_OUTER:.+]], #[[TO_C_OUTER:.+]]>
+; CHECK-DAG: #llvm.di_subprogram<{{.*}}scope = #[[A]],
 
-; CHECK-DAG: #[[TO_B:.+]] = #llvm.di_derived_type<{{.*}}name = "->B", {{.*}}baseType = #[[B]]
-; CHECK-DAG: #[[C_OUTER:.+]] = #llvm.di_composite_type<{{.*}}name = "C", {{.*}}scope = #[[A_SELF]], {{.*}}elements = #[[TO_B]]
-; CHECK-DAG: #[[TO_C:.+]] = #llvm.di_derived_type<{{.*}}name = "->C", {{.*}}baseType = #[[C_OUTER]]
-; CHECK-DAG: #[[A:.+]] = #llvm.di_composite_type<{{.*}}recId = [[A_RECID]], {{.*}}name = "A", {{.*}}elements = #[[TO_B]], #[[TO_C]]
+; CHECK-DAG: #[[TO_B_OUTER]] = #llvm.di_derived_type<{{.*}}name = "->B", {{.*}}baseType = #[[B_OUTER:.+]]>
+; CHECK-DAG: #[[B_OUTER]] = #llvm.di_composite_type<{{.*}}recId = [[B_RECID:.+]], {{.*}}name = "B", {{.*}}elements = #[[TO_C_INNER:.+]]>
+; CHECK-DAG: #[[TO_C_INNER]] = #llvm.di_derived_type<{{.*}}name = "->C", {{.*}}baseType = #[[C_INNER:.+]]>
+; CHECK-DAG: #[[C_INNER]] = #llvm.di_composite_type<{{.*}}name = "C", {{.*}}elements = #[[TO_B_SELF:.+]]>
+; CHECK-DAG: #[[TO_B_SELF]] = #llvm.di_derived_type<{{.*}}name = "->B", {{.*}}baseType = #[[B_SELF:.+]]>
+; CHECK-DAG: #[[B_SELF]] = #llvm.di_composite_type<{{.*}}recId = [[B_RECID]]>
 
-; CHECK-DAG: #llvm.di_subprogram<{{.*}}scope = #[[A]],
+; CHECK-DAG: #[[TO_C_OUTER]] = #llvm.di_derived_type<{{.*}}name = "->C", {{.*}}baseType = #[[C_OUTER:.+]]>
+; CHECK-DAG: #[[C_OUTER]] = #llvm.di_composite_type<{{.*}}name = "C", {{.*}}elements = #[[TO_B_OUTER]]>
 
 define void @class_field(ptr %arg1) !dbg !18 {
   ret void
@@ -688,8 +687,8 @@ define void @class_field(ptr %arg1) !dbg !18 {
 !2 = !DIFile(filename: "debug-info.ll", directory: "/")
 
 !3 = !DICompositeType(tag: DW_TAG_class_type, name: "A", file: !2, line: 42, flags: DIFlagTypePassByReference | DIFlagNonTrivial, elements: !4)
-!5 = !DICompositeType(tag: DW_TAG_class_type, name: "B", scope: !3, file: !2, line: 42, flags: DIFlagTypePassByReference | DIFlagNonTrivial, elements: !10)
-!6 = !DICompositeType(tag: DW_TAG_class_type, name: "C", scope: !3, file: !2, line: 42, flags: DIFlagTypePassByReference | DIFlagNonTrivial, elements: !9)
+!5 = !DICompositeType(tag: DW_TAG_class_type, name: "B", file: !2, line: 42, flags: DIFlagTypePassByReference | DIFlagNonTrivial, elements: !10)
+!6 = !DICompositeType(tag: DW_TAG_class_type, name: "C", file: !2, line: 42, flags: DIFlagTypePassByReference | DIFlagNonTrivial, elements: !9)
 
 !7 = !DIDerivedType(tag: DW_TAG_member, name: "->B", file: !2, baseType: !5)
 !8 = !DIDerivedType(tag: DW_TAG_member, name: "->C", file: !2, baseType: !6)



More information about the Mlir-commits mailing list