[mlir][IR] Add support for UnknownLoc to verify-diagnostics

[matthias-springer]

Diagnostics at unknown locations can now be verified with -verify-diagnostics.

Example:

// expected-error@unknown {{something went wrong}}

Also clean up some MemRefToLLVM conversion tests that had to redirect all errors to stdout in order to FileCheck them. All of those tests can now be stored in a single invalid.mlir. That was not possible before.

[llvmbot]

@llvm/pr-subscribers-mlir

@llvm/pr-subscribers-mlir-core

Author: Matthias Springer (matthias-springer)

Changes

Diagnostics at unknown locations can now be verified with -verify-diagnostics.

Example:

// expected-error@<!-- -->unknown {{something went wrong}}

Also clean up some MemRefToLLVM conversion tests that had to redirect all errors to stdout in order to FileCheck them.

---

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

5 Files Affected:

• (modified) mlir/include/mlir/IR/Diagnostics.h (+2-2)
• (modified) mlir/lib/IR/Diagnostics.cpp (+39-27)
• (removed) mlir/test/Conversion/MemRefToLLVM/invalid-uint.mlir (-8)
• (modified) mlir/test/Conversion/MemRefToLLVM/invalid.mlir (+29-5)
• (removed) mlir/test/Conversion/MemRefToLLVM/issue-70160.mlir (-15)

diff --git a/mlir/include/mlir/IR/Diagnostics.h b/mlir/include/mlir/IR/Diagnostics.h
index 36c433c63b26d..59bed71e4db88 100644
--- a/mlir/include/mlir/IR/Diagnostics.h
+++ b/mlir/include/mlir/IR/Diagnostics.h
@@ -640,8 +640,8 @@ class SourceMgrDiagnosticVerifierHandler : public SourceMgrDiagnosticHandler {
   /// Process a single diagnostic.
   void process(Diagnostic &diag);
 
-  /// Process a FileLineColLoc diagnostic.
-  void process(FileLineColLoc loc, StringRef msg, DiagnosticSeverity kind);
+  /// Process a LocationAttr diagnostic.
+  void process(LocationAttr loc, StringRef msg, DiagnosticSeverity kind);
 
   std::unique_ptr<detail::SourceMgrDiagnosticVerifierHandlerImpl> impl;
 };
diff --git a/mlir/lib/IR/Diagnostics.cpp b/mlir/lib/IR/Diagnostics.cpp
index 19b32120f5890..b699e396f6577 100644
--- a/mlir/lib/IR/Diagnostics.cpp
+++ b/mlir/lib/IR/Diagnostics.cpp
@@ -678,10 +678,13 @@ struct SourceMgrDiagnosticVerifierHandlerImpl {
   /// A list of expected diagnostics for each buffer of the source manager.
   llvm::StringMap<SmallVector<ExpectedDiag, 2>> expectedDiagsPerFile;
 
+  /// A list of expected diagnostics with unknown locations.
+  SmallVector<ExpectedDiag, 2> expectedUnknownLocDiags;
+
   /// Regex to match the expected diagnostics format.
   llvm::Regex expected =
       llvm::Regex("expected-(error|note|remark|warning)(-re)? "
-                  "*(@([+-][0-9]+|above|below))? *{{(.*)}}$");
+                  "*(@([+-][0-9]+|above|below|unknown))? *{{(.*)}}$");
 };
 } // namespace detail
 } // namespace mlir
@@ -774,6 +777,11 @@ SourceMgrDiagnosticVerifierHandlerImpl::computeExpectedDiags(
           record.lineNo += offset;
         else
           record.lineNo -= offset;
+      } else if (offsetMatch.consume_front("unknown")) {
+        // This is matching unknown locations.
+        record.fileLoc = SMLoc();
+        expectedUnknownLocDiags.emplace_back(std::move(record));
+        continue;
       } else if (offsetMatch.consume_front("above")) {
         // If the designator applies 'above' we add it to the last non
         // designator line.
@@ -828,43 +836,45 @@ SourceMgrDiagnosticVerifierHandler::~SourceMgrDiagnosticVerifierHandler() {
 /// verified correctly, failure otherwise.
 LogicalResult SourceMgrDiagnosticVerifierHandler::verify() {
   // Verify that all expected errors were seen.
-  for (auto &expectedDiagsPair : impl->expectedDiagsPerFile) {
-    for (auto &err : expectedDiagsPair.second) {
-      if (err.matched)
-        continue;
+  auto checkExpectedDiags = [&](ExpectedDiag &err) {
+    if (!err.matched)
       impl->status =
           err.emitError(os, mgr,
                         "expected " + getDiagKindStr(err.kind) + " \"" +
                             err.substring + "\" was not produced");
-    }
-  }
+  };
+  for (auto &expectedDiagsPair : impl->expectedDiagsPerFile)
+    for (auto &err : expectedDiagsPair.second)
+      checkExpectedDiags(err);
+  for (auto &err : impl->expectedUnknownLocDiags)
+    checkExpectedDiags(err);
   impl->expectedDiagsPerFile.clear();
   return impl->status;
 }
 
 /// Process a single diagnostic.
 void SourceMgrDiagnosticVerifierHandler::process(Diagnostic &diag) {
-  auto kind = diag.getSeverity();
-
-  // Process a FileLineColLoc.
-  if (auto fileLoc = diag.getLocation()->findInstanceOf<FileLineColLoc>())
-    return process(fileLoc, diag.str(), kind);
-
-  emitDiagnostic(diag.getLocation(),
-                 "unexpected " + getDiagKindStr(kind) + ": " + diag.str(),
-                 DiagnosticSeverity::Error);
-  impl->status = failure();
+  return process(diag.getLocation(), diag.str(), diag.getSeverity());
 }
 
-/// Process a FileLineColLoc diagnostic.
-void SourceMgrDiagnosticVerifierHandler::process(FileLineColLoc loc,
+/// Process a diagnostic at a certain location.
+void SourceMgrDiagnosticVerifierHandler::process(LocationAttr loc,
                                                  StringRef msg,
                                                  DiagnosticSeverity kind) {
-  // Get the expected diagnostics for this file.
-  auto diags = impl->getExpectedDiags(loc.getFilename());
-  if (!diags) {
-    diags = impl->computeExpectedDiags(os, mgr,
-                                       getBufferForFile(loc.getFilename()));
+  FileLineColLoc fileLoc = loc.findInstanceOf<FileLineColLoc>();
+  MutableArrayRef<ExpectedDiag> diags;
+
+  if (fileLoc) {
+    // Get the expected diagnostics for this file.
+    if (auto maybeDiags = impl->getExpectedDiags(fileLoc.getFilename())) {
+      diags = *maybeDiags;
+    } else {
+      diags = impl->computeExpectedDiags(
+          os, mgr, getBufferForFile(fileLoc.getFilename()));
+    }
+  } else {
+    // Get all expected diagnostics at unknown locations.
+    diags = impl->expectedUnknownLocDiags;
   }
 
   // Search for a matching expected diagnostic.
@@ -872,9 +882,11 @@ void SourceMgrDiagnosticVerifierHandler::process(FileLineColLoc loc,
   ExpectedDiag *nearMiss = nullptr;
 
   // If this was an expected error, remember that we saw it and return.
-  unsigned line = loc.getLine();
-  for (auto &e : *diags) {
-    if (line == e.lineNo && e.match(msg)) {
+  for (auto &e : diags) {
+    // File line must match (unless it's an unknown location).
+    if (fileLoc && fileLoc.getLine() != e.lineNo)
+      continue;
+    if (e.match(msg)) {
       if (e.kind == kind) {
         e.matched = true;
         return;
diff --git a/mlir/test/Conversion/MemRefToLLVM/invalid-uint.mlir b/mlir/test/Conversion/MemRefToLLVM/invalid-uint.mlir
deleted file mode 100644
index 7e94677ebbdd7..0000000000000
--- a/mlir/test/Conversion/MemRefToLLVM/invalid-uint.mlir
+++ /dev/null
@@ -1,8 +0,0 @@
-// RUN: mlir-opt %s -finalize-memref-to-llvm -verify-diagnostics
-
-// CHECK-LABEL: @invalid_int_conversion
-func.func @invalid_int_conversion() {
-     // expected-error@+1 {{conversion of memref memory space 1 : ui64 to integer address space failed. Consider adding memory space conversions.}}
-     %alloc = memref.alloc() {alignment = 64 : i64} : memref<10xf32, 1 : ui64> 
-    return
-}
diff --git a/mlir/test/Conversion/MemRefToLLVM/invalid.mlir b/mlir/test/Conversion/MemRefToLLVM/invalid.mlir
index 31bfa7a44a133..61c67005a08fc 100644
--- a/mlir/test/Conversion/MemRefToLLVM/invalid.mlir
+++ b/mlir/test/Conversion/MemRefToLLVM/invalid.mlir
@@ -1,15 +1,15 @@
-// RUN: mlir-opt %s -finalize-memref-to-llvm 2>&1 | FileCheck %s
-// Since the error is at an unknown location, we use FileCheck instead of
-// -verify-diagnostics here
+// RUN: mlir-opt %s -finalize-memref-to-llvm -split-input-file -verify-diagnostics | FileCheck %s
 
-// CHECK: redefinition of reserved function 'malloc' of different type '!llvm.func<void (i64)>' is prohibited
+// expected-error@+1{{redefinition of reserved function 'malloc' of different type '!llvm.func<void (i64)>' is prohibited}}
 llvm.func @malloc(i64)
 func.func @redef_reserved() {
     %alloc = memref.alloc() : memref<1024x64xf32, 1>
     llvm.return
 }
 
-// CHECK: conversion of memref memory space "foo" to integer address space failed. Consider adding memory space conversions.
+// -----
+
+// expected-error@unknown{{conversion of memref memory space "foo" to integer address space failed. Consider adding memory space conversions.}}
 // CHECK-LABEL: @bad_address_space
 func.func @bad_address_space(%a: memref<2xindex, "foo">) {
     %c0 = arith.constant 0 : index
@@ -17,3 +17,27 @@ func.func @bad_address_space(%a: memref<2xindex, "foo">) {
     memref.store %c0, %a[%c0] : memref<2xindex, "foo">
     return
 }
+
+// -----
+
+// CHECK-LABEL: @invalid_int_conversion
+func.func @invalid_int_conversion() {
+     // expected-error@+1 {{conversion of memref memory space 1 : ui64 to integer address space failed. Consider adding memory space conversions.}}
+     %alloc = memref.alloc() {alignment = 64 : i64} : memref<10xf32, 1 : ui64> 
+    return
+}
+
+// -----
+
+// expected-error@unknown{{conversion of memref memory space #gpu.address_space<workgroup> to integer address space failed. Consider adding memory space conversions}}
+// CHECK-LABEL: @issue_70160
+func.func @issue_70160() {
+  // expected-error@+1{{conversion of memref memory space #gpu.address_space<workgroup> to integer address space failed. Consider adding memory space conversions}}
+  %alloc = memref.alloc() : memref<1x32x33xi32, #gpu.address_space<workgroup>>
+  %alloc1 = memref.alloc() : memref<i32>
+  %c0 = arith.constant 0 : index
+  // CHECK: memref.load
+  %0 = memref.load %alloc[%c0, %c0, %c0] : memref<1x32x33xi32, #gpu.address_space<workgroup>>
+  memref.store %0, %alloc1[] : memref<i32>
+  func.return
+}
diff --git a/mlir/test/Conversion/MemRefToLLVM/issue-70160.mlir b/mlir/test/Conversion/MemRefToLLVM/issue-70160.mlir
deleted file mode 100644
index 6970e5f413984..0000000000000
--- a/mlir/test/Conversion/MemRefToLLVM/issue-70160.mlir
+++ /dev/null
@@ -1,15 +0,0 @@
-// RUN: mlir-opt %s -finalize-memref-to-llvm 2>&1 | FileCheck %s
-// Since the error is at an unknown location, we use FileCheck instead of
-// -verify-diagnostics here
-
-// CHECK: conversion of memref memory space #gpu.address_space<workgroup> to integer address space failed. Consider adding memory space conversions
-// CHECK-LABEL: @issue_70160
-func.func @issue_70160() {
-  %alloc = memref.alloc() : memref<1x32x33xi32, #gpu.address_space<workgroup>>
-  %alloc1 = memref.alloc() : memref<i32>
-  %c0 = arith.constant 0 : index
-  // CHECK: memref.load
-  %0 = memref.load %alloc[%c0, %c0, %c0] : memref<1x32x33xi32, #gpu.address_space<workgroup>>
-  memref.store %0, %alloc1[] : memref<i32>
-  func.return
-}