Use more references in Diagnostic handling (NFC)

[mehdi_amini]

Summary

This saves some copies of non-trivial objects.

[rriddle]

mlir/include/mlir/IR/Diagnostics.h

433–436

This should also now indicate that diag may be consumed during emission, i.e. you can't rely on it being the same after this call.

[jpienaar]

Internally here we were moving these, were the additional copies outside here/from user code invocation?

mlir/include/mlir/IR/Diagnostics.h

433–436

Should we (and this would be more invasive ...) make this as rvalue to make it clear at call site that this is the case? Ref does indicate it can be modified but sounds like the expectation of the user should be more akin to what post move variable is.

[mehdi_amini]

Use r-values in the APIs (should we also update the handlers?)

[rriddle]

Use of r-value on user facing API LG.

In D117525#3249965, @mehdi_amini wrote:

should we also update the handlers?

A specific handler isn't guaranteed to completely handle a diagnostic, and can signal to the diagnostic engine that the diagnostic should be passed on to the next registered handler.

mlir/include/mlir/IR/Diagnostics.h

public:
 
   /// Create a new inflight diagnostic with the given location and severity.
   InFlightDiagnostic emit(Location loc, DiagnosticSeverity severity) {
     assert(severity != DiagnosticSeverity::Note &&
            "notes should not be emitted directly");
     return InFlightDiagnostic(this, Diagnostic(loc, severity));
   }
 
   /// Emit a diagnostic using the registered issue handler if present, or with
-  /// the default behavior if not.
-  void emit(Diagnostic diag);
+  /// the default behavior if not. The diagnostic instance is consumed in the
+  /// process.
+  void emit(Diagnostic &&diag);
 
 private:
   friend class MLIRContextImpl;
   DiagnosticEngine();
 
   /// The internal implementation of the DiagnosticEngine.
   std::unique_ptr<detail::DiagnosticEngineImpl> impl;
 };

mlir/lib/IR/Diagnostics.cpp

 // DiagnosticEngineImpl
 //===----------------------------------------------------------------------===//
 
 namespace mlir {
 namespace detail {
 struct DiagnosticEngineImpl {
   /// Emit a diagnostic using the registered issue handle if present, or with
   /// the default behavior if not.
-  void emit(Diagnostic diag);
+  void emit(Diagnostic &&diag);
 
   /// A mutex to ensure that diagnostics emission is thread-safe.
   llvm::sys::SmartMutex<true> mutex;
 
   /// These are the handlers used to report diagnostics.
   llvm::SmallMapVector<DiagnosticEngine::HandlerID, DiagnosticEngine::HandlerTy,
                        2>
       handlers;
 
   /// This is a unique identifier counter for diagnostic handlers in the
   /// context. This id starts at 1 to allow for 0 to be used as a sentinel.
   DiagnosticEngine::HandlerID uniqueHandlerId = 1;
 };
 } // namespace detail
 } // namespace mlir
 
 /// Emit a diagnostic using the registered issue handle if present, or with
 /// the default behavior if not.
-void DiagnosticEngineImpl::emit(Diagnostic diag) {
+void DiagnosticEngineImpl::emit(Diagnostic &&diag) {
   llvm::sys::SmartScopedLock<true> lock(mutex);
 
   // Try to process the given diagnostic on one of the registered handlers.
   // Handlers are walked in reverse order, so that the most recent handler is
   // processed first.
   for (auto &handlerIt : llvm::reverse(handlers))
     if (succeeded(handlerIt.second(diag)))
       return;

 /// Erase the registered diagnostic handler with the given identifier.
 void DiagnosticEngine::eraseHandler(HandlerID handlerID) {
   llvm::sys::SmartScopedLock<true> lock(impl->mutex);
   impl->handlers.erase(handlerID);
 }
 
 /// Emit a diagnostic using the registered issue handler if present, or with
 /// the default behavior if not.
-void DiagnosticEngine::emit(Diagnostic diag) {
+void DiagnosticEngine::emit(Diagnostic &&diag) {
   assert(diag.getSeverity() != DiagnosticSeverity::Note &&
          "notes should not be emitted directly");
   impl->emit(std::move(diag));
 }
 
 /// Helper function used to emit a diagnostic with an optionally empty twine
 /// message. If the message is empty, then it is not inserted into the
 /// diagnostic.
~ParallelDiagnosticHandlerImpl() override {
     // Erase this handler from the context.
     context->getDiagEngine().eraseHandler(handlerID);
 
     // Early exit if there are no diagnostics, this is the common case.
     if (diagnostics.empty())
       return;
 
     // Emit the diagnostics back to the context.
-    emitDiagnostics([&](Diagnostic diag) {
+    emitDiagnostics([&](Diagnostic &diag) {
       return context->getDiagEngine().emit(std::move(diag));
     });
   }
 
   /// Utility method to emit any held diagnostics.
-  void emitDiagnostics(llvm::function_ref<void(Diagnostic)> emitFn) const {
+  void emitDiagnostics(llvm::function_ref<void(Diagnostic &)> emitFn) const {
     // Stable sort all of the diagnostics that were emitted. This creates a
     // deterministic ordering for the diagnostics based upon which order id they
     // were emitted for.
     std::stable_sort(diagnostics.begin(), diagnostics.end());
 
     // Emit each diagnostic to the context again.
     for (ThreadDiagnostic &diag : diagnostics)
-      emitFn(std::move(diag.diag));
+      emitFn(diag.diag);
   }
 
   /// Set the order id for the current thread.
   void setOrderIDForThread(size_t orderID) {
     uint64_t tid = llvm::get_threadid();
     llvm::sys::SmartScopedLock<true> lock(mutex);
     threadToOrderID[tid] = orderID;
   }