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

[llvmlistbot at llvm.org]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-mlir

Author: Billy Zhu (zyx-billy)

<details>
<summary>Changes</summary>

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.

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


4 Files Affected:

- (modified) mlir/lib/Pass/Pass.cpp (+6-3) 
- (modified) mlir/test/Pass/crash-recovery-dynamic-failure.mlir (+1-1) 
- (added) mlir/test/Pass/full_diagnostics.mlir (+17) 
- (modified) mlir/test/lib/Pass/TestPassManager.cpp (+11-1) 


``````````diff
diff --git a/mlir/lib/Pass/Pass.cpp b/mlir/lib/Pass/Pass.cpp
index 83b46e8f5680a0..90fd9a713e41d7 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,6 +799,7 @@ 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);
+  std::atomic<bool> hasFailure = false;
   auto processFn = [&](OpPMInfo &opInfo) {
     // Find an executor for this operation.
     auto it = llvm::find_if(activePMs, [](std::atomic<bool> &isActive) {
@@ -812,14 +813,16 @@ 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;
   };
+  parallelForEach(context, opInfos, processFn);
 
   // 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.

``````````

</details>


https://github.com/llvm/llvm-project/pull/110311