[mlir] Expand error message to include unregistered dialects.

[jpienaar]

It is possible to load unregistered dialects, this can result in a confusing error message. Mark unregistered but loaded dialects.

This is currently done as a merged list as that is most concise but requires some additional preprocessing (did merge sort given the other two lists are, could do it shorter and probably at not too much extra cost if I just used SetVectors - so alternative which uses less code and may be preferred as performance not critical here).

[llvmbot]

@llvm/pr-subscribers-mlir

@llvm/pr-subscribers-mlir-core

Author: Jacques Pienaar (jpienaar)

Changes

It is possible to load unregistered dialects, this can result in a confusing error message. Mark unregistered but loaded dialects.

This is currently done as a merged list as that is most concise but requires some additional preprocessing (did merge sort given the other two lists are, could do it shorter and probably at not too much extra cost if I just used SetVectors - so alternative which uses less code and may be preferred as performance not critical here).

---

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

1 Files Affected:

• (modified) mlir/lib/AsmParser/Parser.cpp (+39-3)

diff --git a/mlir/lib/AsmParser/Parser.cpp b/mlir/lib/AsmParser/Parser.cpp
index 435ff713a1b29..86ffe41092547 100644
--- a/mlir/lib/AsmParser/Parser.cpp
+++ b/mlir/lib/AsmParser/Parser.cpp
@@ -2077,9 +2077,45 @@ OperationParser::parseCustomOperation(ArrayRef<ResultRecord> resultIDs) {
         diag << " (tried '" << opName << "' as well)";
       auto &note = diag.attachNote();
       note << "Registered dialects: ";
-      llvm::interleaveComma(getContext()->getAvailableDialects(), note,
-                            [&](StringRef dialect) { note << dialect; });
-      note << " ; for more info on dialect registration see "
+      std::vector<StringRef> registered = getContext()->getAvailableDialects();
+      auto loaded = getContext()->getLoadedDialects();
+
+      // Merge the sorted lists of registered and loaded dialects.
+      SmallVector<std::pair<StringRef, bool>> mergedDialects;
+      auto regIt = registered.begin(), regEnd = registered.end();
+      auto loadIt = loaded.rbegin(), loadEnd = loaded.rend();
+      bool isRegistered = false;
+      bool isOnlyLoaded = true;
+      while (regIt != regEnd && loadIt != loadEnd) {
+        StringRef reg = *regIt;
+        StringRef load = (*loadIt)->getNamespace();
+        if (reg < load) {
+          mergedDialects.emplace_back(*regIt++, isRegistered);
+        } else if (load < reg) {
+          mergedDialects.emplace_back(load, isOnlyLoaded);
+          loadIt++;
+        } else {
+          mergedDialects.emplace_back(*regIt++, isRegistered);
+          loadIt++;
+        }
+      }
+      for (; regIt != regEnd; ++regIt)
+        mergedDialects.emplace_back(*regIt, isRegistered);
+      for (; loadIt != loadEnd; ++loadIt)
+        mergedDialects.emplace_back((*loadIt)->getNamespace(), isOnlyLoaded);
+
+      bool loadedUnregistered = false;
+      llvm::interleaveComma(mergedDialects, note, [&](auto &pair) {
+        note << pair.first;
+        if (pair.second) {
+          loadedUnregistered = true;
+          note << " (*)";
+        }
+      });
+      note << " ";
+      if (loadedUnregistered)
+        note << "(* corresponding to loaded but unregistered dialects)";
+      note << "; for more info on dialect registration see "
               "https://mlir.llvm.org/getting_started/Faq/"
               "#registered-loaded-dependent-whats-up-with-dialects-management";
       return nullptr;

[joker-eph]

I'm a bit confused when does it matter? If the dialect is loaded for the op you're looking for, you'll never have this error message anyway?

[jpienaar]

If your input contains some registered, some unregistered (loaded & unloaded) dialects, then the error returned is confusing: you see list of registered dialects and it failed to parse, but it gets further than one would expect given the list of dialects reported ("why did it fail on X post parsing A but neither A nor X is registered?"). It really matters what is or could be loaded instead of registered as to why parsing failed.

[now it may also be an oversight that folks have loaded but unregistered dialects, which is why I still mark it]

[joker-eph]

now it may also be an oversight that folks have loaded but unregistered dialects

No that's fine with me, registration is totally optional.
I'm skeptical about this part: "why did it fail on X post parsing A but neither A nor X is registered?", but this change may not hurt either, so why not...