[Mlir-commits] [mlir] 3d44f48 - [mlir][Diagnostics] Don't print note source line if it is the same as the previous diagnostic

[River Riddle]

Author: River Riddle
Date: 2020-03-29T21:43:44-07:00
New Revision: 3d44f48edc271dc2e1f68d5281f7f4bd11949340

URL: https://github.com/llvm/llvm-project/commit/3d44f48edc271dc2e1f68d5281f7f4bd11949340
DIFF: https://github.com/llvm/llvm-project/commit/3d44f48edc271dc2e1f68d5281f7f4bd11949340.diff

LOG: [mlir][Diagnostics] Don't print note source line if it is the same as the previous diagnostic

Summary: This revision updates the SourceMgrDiagnosticHandler to not print the source location of a note if it is the same location as the previously printed diagnostic. This helps avoid redundancy, and potential confusion, when looking at the diagnostic output.

Differential Revision: https://reviews.llvm.org/D76787

Added: 
    

Modified: 
    mlir/include/mlir/IR/Diagnostics.h
    mlir/lib/IR/Diagnostics.cpp
    mlir/test/IR/diagnostic-handler.mlir

Removed: 
    


################################################################################
diff  --git a/mlir/include/mlir/IR/Diagnostics.h b/mlir/include/mlir/IR/Diagnostics.h
index f366935d61f9..20fd366398d3 100644
--- a/mlir/include/mlir/IR/Diagnostics.h
+++ b/mlir/include/mlir/IR/Diagnostics.h
@@ -547,7 +547,8 @@ class SourceMgrDiagnosticHandler : public ScopedDiagnosticHandler {
   ~SourceMgrDiagnosticHandler();
 
   /// Emit the given diagnostic information with the held source manager.
-  void emitDiagnostic(Location loc, Twine message, DiagnosticSeverity kind);
+  void emitDiagnostic(Location loc, Twine message, DiagnosticSeverity kind,
+                      bool displaySourceLine = true);
 
 protected:
   /// Emit the given diagnostic with the held source manager.

diff  --git a/mlir/lib/IR/Diagnostics.cpp b/mlir/lib/IR/Diagnostics.cpp
index 3e8999fcf8bb..1acb7e7f5931 100644
--- a/mlir/lib/IR/Diagnostics.cpp
+++ b/mlir/lib/IR/Diagnostics.cpp
@@ -435,7 +435,8 @@ SourceMgrDiagnosticHandler::SourceMgrDiagnosticHandler(llvm::SourceMgr &mgr,
 SourceMgrDiagnosticHandler::~SourceMgrDiagnosticHandler() {}
 
 void SourceMgrDiagnosticHandler::emitDiagnostic(Location loc, Twine message,
-                                                DiagnosticSeverity kind) {
+                                                DiagnosticSeverity kind,
+                                                bool displaySourceLine) {
   // Extract a file location from this loc.
   auto fileLoc = getFileLineColLoc(loc);
 
@@ -449,41 +450,52 @@ void SourceMgrDiagnosticHandler::emitDiagnostic(Location loc, Twine message,
     return mgr.PrintMessage(os, llvm::SMLoc(), getDiagKind(kind), strOS.str());
   }
 
-  // Otherwise, try to convert the file location to an SMLoc.
-  auto smloc = convertLocToSMLoc(*fileLoc);
-  if (smloc.isValid())
-    return mgr.PrintMessage(os, smloc, getDiagKind(kind), message);
+  // Otherwise if we are displaying the source line, try to convert the file
+  // location to an SMLoc.
+  if (displaySourceLine) {
+    auto smloc = convertLocToSMLoc(*fileLoc);
+    if (smloc.isValid())
+      return mgr.PrintMessage(os, smloc, getDiagKind(kind), message);
+  }
 
   // If the conversion was unsuccessful, create a diagnostic with the file
-  // information.
-  llvm::SMDiagnostic diag(fileLoc->getFilename(), getDiagKind(kind),
-                          message.str());
+  // information. We manually combine the line and column to avoid asserts in
+  // the constructor of SMDiagnostic that takes a location.
+  std::string locStr;
+  llvm::raw_string_ostream locOS(locStr);
+  locOS << fileLoc->getFilename() << ":" << fileLoc->getLine() << ":"
+        << fileLoc->getColumn();
+  llvm::SMDiagnostic diag(locOS.str(), getDiagKind(kind), message.str());
   diag.print(nullptr, os);
 }
 
 /// Emit the given diagnostic with the held source manager.
 void SourceMgrDiagnosticHandler::emitDiagnostic(Diagnostic &diag) {
   // Emit the diagnostic.
-  auto loc = diag.getLocation();
+  Location loc = diag.getLocation();
   emitDiagnostic(loc, diag.str(), diag.getSeverity());
 
   // If the diagnostic location was a call site location, then print the call
   // stack as well.
   if (auto callLoc = getCallSiteLoc(loc)) {
     // Print the call stack while valid, or until the limit is reached.
-    Location callerLoc = callLoc->getCaller();
+    loc = callLoc->getCaller();
     for (unsigned curDepth = 0; curDepth < callStackLimit; ++curDepth) {
-      emitDiagnostic(callerLoc, "called from", DiagnosticSeverity::Note);
-      callLoc = getCallSiteLoc(callerLoc);
-      if (!callLoc)
+      emitDiagnostic(loc, "called from", DiagnosticSeverity::Note);
+      if ((callLoc = getCallSiteLoc(loc)))
+        loc = callLoc->getCaller();
+      else
         break;
-      callerLoc = callLoc->getCaller();
     }
   }
 
-  // Emit each of the notes.
-  for (auto &note : diag.getNotes())
-    emitDiagnostic(note.getLocation(), note.str(), note.getSeverity());
+  // Emit each of the notes. Only display the source code if the location is
+  // 
diff erent from the previous location.
+  for (auto &note : diag.getNotes()) {
+    emitDiagnostic(note.getLocation(), note.str(), note.getSeverity(),
+                   /*displaySourceLine=*/loc != note.getLocation());
+    loc = note.getLocation();
+  }
 }
 
 /// Get a memory buffer for the given file, or nullptr if one is not found.

diff  --git a/mlir/test/IR/diagnostic-handler.mlir b/mlir/test/IR/diagnostic-handler.mlir
index d3108e33f78f..198d5e84e119 100644
--- a/mlir/test/IR/diagnostic-handler.mlir
+++ b/mlir/test/IR/diagnostic-handler.mlir
@@ -5,9 +5,9 @@
 
 // Emit the first available call stack in the fused location.
 func @constant_out_of_range() {
-  // CHECK: mysource1: error: 'std.constant' op requires attribute's type ('i64') to match op's return type ('i1')
-  // CHECK-NEXT: mysource2: note: called from
-  // CHECK-NEXT: mysource3: note: called from
+  // CHECK: mysource1:0:0: error: 'std.constant' op requires attribute's type ('i64') to match op's return type ('i1')
+  // CHECK-NEXT: mysource2:1:0: note: called from
+  // CHECK-NEXT: mysource3:2:0: note: called from
   %x = "std.constant"() {value = 100} : () -> i1 loc(fused["bar", callsite("foo"("mysource1":0:0) at callsite("mysource2":1:0 at "mysource3":2:0))])
   return
 }