[Mlir-commits] [mlir] 5b21fd2 - [MLIR][Pass] Full & deterministic diagnostics (#110311)

[llvmlistbot at llvm.org]

Author: Billy Zhu
Date: 2024-10-01T19:07:52-07:00
New Revision: 5b21fd298cb4fc2042a95ffb9284b778f8504e04

URL: https://github.com/llvm/llvm-project/commit/5b21fd298cb4fc2042a95ffb9284b778f8504e04
DIFF: https://github.com/llvm/llvm-project/commit/5b21fd298cb4fc2042a95ffb9284b778f8504e04.diff

LOG: [MLIR][Pass] Full & deterministic diagnostics (#110311)

Today, when the pass infra schedules a pass/nested-pipeline on a set of
ops, it exits early as soon as it fails on one of the ops. This leads to
non-exhaustive, and more importantly, non-deterministic error reporting
(under async).

This PR removes the early termination behavior so that all ops have a
chance to run through the current pass/nested-pipeline, and all errors
are reported (async diagnostics are already ordered). This guarantees
deterministic & full error reporting. As a result, it's also no longer
necessary to -split-input-file with one error per split when testing
with -verify-diagnostics.

Added: 
    mlir/test/Pass/full_diagnostics.mlir

Modified: 
    mlir/lib/Pass/Pass.cpp
    mlir/test/Pass/crash-recovery-dynamic-failure.mlir
    mlir/test/lib/Pass/TestPassManager.cpp

Removed: 
    


################################################################################
diff  --git a/mlir/lib/Pass/Pass.cpp b/mlir/lib/Pass/Pass.cpp
index 83b46e8f5680a0..6fd51c1e3cb538 100644
--- a/mlir/lib/Pass/Pass.cpp
+++ b/mlir/lib/Pass/Pass.cpp
@@ -732,7 +732,7 @@ void OpToOpPassAdaptor::runOnOperationImpl(bool verifyPasses) {
         unsigned initGeneration = mgr->impl->initializationGeneration;
         if (failed(runPipeline(*mgr, &op, am.nest(&op), verifyPasses,
                                initGeneration, instrumentor, &parentInfo)))
-          return signalPassFailure();
+          signalPassFailure();
       }
     }
   }
@@ -799,7 +799,8 @@ void OpToOpPassAdaptor::runOnOperationAsyncImpl(bool verifyPasses) {
   // An atomic failure variable for the async executors.
   std::vector<std::atomic<bool>> activePMs(asyncExecutors.size());
   std::fill(activePMs.begin(), activePMs.end(), false);
-  auto processFn = [&](OpPMInfo &opInfo) {
+  std::atomic<bool> hasFailure = false;
+  parallelForEach(context, opInfos, [&](OpPMInfo &opInfo) {
     // Find an executor for this operation.
     auto it = llvm::find_if(activePMs, [](std::atomic<bool> &isActive) {
       bool expectedInactive = false;
@@ -812,14 +813,15 @@ void OpToOpPassAdaptor::runOnOperationAsyncImpl(bool verifyPasses) {
     LogicalResult pipelineResult = runPipeline(
         pm, opInfo.op, opInfo.am, verifyPasses,
         pm.impl->initializationGeneration, instrumentor, &parentInfo);
+    if (failed(pipelineResult))
+      hasFailure.store(true);
 
     // Reset the active bit for this pass manager.
     activePMs[pmIndex].store(false);
-    return pipelineResult;
-  };
+  });
 
   // Signal a failure if any of the executors failed.
-  if (failed(failableParallelForEach(context, opInfos, processFn)))
+  if (hasFailure)
     signalPassFailure();
 }
 

diff  --git a/mlir/test/Pass/crash-recovery-dynamic-failure.mlir b/mlir/test/Pass/crash-recovery-dynamic-failure.mlir
index cea4f6a79963bd..6d2bd696391086 100644
--- a/mlir/test/Pass/crash-recovery-dynamic-failure.mlir
+++ b/mlir/test/Pass/crash-recovery-dynamic-failure.mlir
@@ -15,4 +15,4 @@ module @inner_mod1 {
 // REPRO_LOCAL_DYNAMIC_FAILURE: module @inner_mod1
 // REPRO_LOCAL_DYNAMIC_FAILURE: module @foo {
 
-// REPRO_LOCAL_DYNAMIC_FAILURE: pipeline: "builtin.module(builtin.module(test-pass-failure))"
+// REPRO_LOCAL_DYNAMIC_FAILURE: pipeline: "builtin.module(builtin.module(test-pass-failure{gen-diagnostics=false}))"

diff  --git a/mlir/test/Pass/full_diagnostics.mlir b/mlir/test/Pass/full_diagnostics.mlir
new file mode 100644
index 00000000000000..881260ce60d321
--- /dev/null
+++ b/mlir/test/Pass/full_diagnostics.mlir
@@ -0,0 +1,17 @@
+// RUN: mlir-opt %s -pass-pipeline='builtin.module(func.func(test-pass-failure{gen-diagnostics}))' -verify-diagnostics
+
+// Test that all errors are reported.
+// expected-error at below {{illegal operation}}
+func.func @TestAlwaysIllegalOperationPass1() {
+  return
+}
+
+// expected-error at below {{illegal operation}}
+func.func @TestAlwaysIllegalOperationPass2() {
+  return
+}
+
+// expected-error at below {{illegal operation}}
+func.func @TestAlwaysIllegalOperationPass3() {
+  return
+}

diff  --git a/mlir/test/lib/Pass/TestPassManager.cpp b/mlir/test/lib/Pass/TestPassManager.cpp
index ee32bec0c79bd4..7afe2109f04db3 100644
--- a/mlir/test/lib/Pass/TestPassManager.cpp
+++ b/mlir/test/lib/Pass/TestPassManager.cpp
@@ -151,11 +151,21 @@ struct TestCrashRecoveryPass
 struct TestFailurePass : public PassWrapper<TestFailurePass, OperationPass<>> {
   MLIR_DEFINE_EXPLICIT_INTERNAL_INLINE_TYPE_ID(TestFailurePass)
 
-  void runOnOperation() final { signalPassFailure(); }
+  TestFailurePass() = default;
+  TestFailurePass(const TestFailurePass &other) : PassWrapper(other) {}
+
+  void runOnOperation() final {
+    signalPassFailure();
+    if (genDiagnostics)
+      mlir::emitError(getOperation()->getLoc(), "illegal operation");
+  }
   StringRef getArgument() const final { return "test-pass-failure"; }
   StringRef getDescription() const final {
     return "Test a pass in the pass manager that always fails";
   }
+
+  Option<bool> genDiagnostics{*this, "gen-diagnostics",
+                              llvm::cl::desc("Generate a diagnostic message")};
 };
 
 /// A test pass that creates an invalid operation in a function body.