Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[mlir][transform] Emit error message with emitSilenceableFailure #86146

Merged

Conversation

srcarroll
Copy link
Contributor

The previous implementation used a notifyMatchFailure to emit failure message inappropriately and then used the emitDefaultSilenceableFailure. This patch changes this to use the more appropriate emitSilenceableFailure with error message. Additionally a failure test has been added.

@llvmbot
Copy link
Collaborator

llvmbot commented Mar 21, 2024

@llvm/pr-subscribers-mlir

@llvm/pr-subscribers-mlir-linalg

Author: None (srcarroll)

Changes

The previous implementation used a notifyMatchFailure to emit failure message inappropriately and then used the emitDefaultSilenceableFailure. This patch changes this to use the more appropriate emitSilenceableFailure with error message. Additionally a failure test has been added.


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

2 Files Affected:

  • (modified) mlir/lib/Dialect/Linalg/TransformOps/LinalgTransformOps.cpp (+4-5)
  • (added) mlir/test/Dialect/Linalg/flatten-unsupported.mlir (+16)
diff --git a/mlir/lib/Dialect/Linalg/TransformOps/LinalgTransformOps.cpp b/mlir/lib/Dialect/Linalg/TransformOps/LinalgTransformOps.cpp
index ecf9983124821a..06acade06d771b 100644
--- a/mlir/lib/Dialect/Linalg/TransformOps/LinalgTransformOps.cpp
+++ b/mlir/lib/Dialect/Linalg/TransformOps/LinalgTransformOps.cpp
@@ -3269,11 +3269,10 @@ DiagnosedSilenceableFailure transform::FlattenElementwiseLinalgOp::applyToOne(
     transform::ApplyToEachResultList &results,
     transform::TransformState &state) {
   rewriter.setInsertionPoint(target);
-  if (!isElementwise(target)) {
-    failed(rewriter.notifyMatchFailure(
-        target, "only elementwise flattening is supported"));
-    return emitDefaultSilenceableFailure(target);
-  }
+  if (!isElementwise(target))
+
+    return mlir::emitSilenceableFailure(target->getLoc())
+           << "only elementwise flattening is supported";
   // If rank <= 1, do nothing
   if (target.getNumLoops() <= 1) {
     results.push_back(target);
diff --git a/mlir/test/Dialect/Linalg/flatten-unsupported.mlir b/mlir/test/Dialect/Linalg/flatten-unsupported.mlir
new file mode 100644
index 00000000000000..476733fc74f5cf
--- /dev/null
+++ b/mlir/test/Dialect/Linalg/flatten-unsupported.mlir
@@ -0,0 +1,16 @@
+// RUN: mlir-opt %s -transform-interpreter -split-input-file -verify-diagnostics
+
+func.func @non_elementwise(%arg0: memref<2x3xf32>, %arg1: memref<3x4xf32>, %arg2: memref<2x4xf32>) {
+  // expected-error @+1 {{only elementwise flattening is supported}}
+  linalg.matmul ins(%arg0, %arg1 : memref<2x3xf32>, memref<3x4xf32>) outs(%arg2: memref<2x4xf32>)
+  return
+}
+
+module attributes {transform.with_named_sequence} {
+  transform.named_sequence @__transform_main(%arg1: !transform.any_op {transform.readonly}) {
+    %0 = transform.structured.match interface{LinalgOp} in %arg1 : (!transform.any_op) -> !transform.any_op
+    %flattened = transform.structured.flatten_elementwise %0
+      : (!transform.any_op) -> !transform.any_op
+    transform.yield
+  }
+}

@srcarroll
Copy link
Contributor Author

I'm still waiting on my local build to test this change

@srcarroll
Copy link
Contributor Author

srcarroll commented Mar 21, 2024

@ftynse As a general rule of thumb, should i even bother waiting for the windows buildkite to finish? often it just hangs for several hours, other times it just fails for reasons irrelevant to the PR, and rarely does it actually finish and pass (for me at least)

@srcarroll
Copy link
Contributor Author

srcarroll commented Mar 21, 2024

need to fix this one too https://github.com/llvm/llvm-project/pull/86146/files#diff-61bfc4a79549b25af3e46ff8002c9a9aea61fb537b565435ba0957e2b53b9adaR3286. just need to figure out a test case to trigger it

@srcarroll
Copy link
Contributor Author

need to fix this one too https://github.com/llvm/llvm-project/pull/86146/files#diff-61bfc4a79549b25af3e46ff8002c9a9aea61fb537b565435ba0957e2b53b9adaR3286. just need to figure out a test case to trigger it

@ftynse this was done after your approval so i'll wait for you thumbs up on this

@ftynse
Copy link
Member

ftynse commented Mar 22, 2024

@ftynse As a general rule of thumb, should i even bother waiting for the windows buildkite to finish? often it just hangs for several hours, other times it just fails for reasons irrelevant to the PR, and rarely does it actually finish and pass (for me at least)

Use your best judgement and be ready to revert if submitting before it passes. I usually wait when changing the build system (Windows/MSVC tends to catch missing dependencies) or ABI-impacting things, but may not wait otherwise. 10 hours is an overkill, but it did pass this time.

@ftynse
Copy link
Member

ftynse commented Mar 22, 2024

need to fix this one too https://github.com/llvm/llvm-project/pull/86146/files#diff-61bfc4a79549b25af3e46ff8002c9a9aea61fb537b565435ba0957e2b53b9adaR3286. just need to figure out a test case to trigger it

Thanks!

@srcarroll srcarroll merged commit bbcfe6f into llvm:main Mar 22, 2024
3 of 4 checks passed
chencha3 pushed a commit to chencha3/llvm-project that referenced this pull request Mar 23, 2024
…lvm#86146)

The previous implementation used a `notifyMatchFailure` to emit failure
message inappropriately and then used the
`emitDefaultSilenceableFailure`. This patch changes this to use the more
appropriate `emitSilenceableFailure` with error message. Additionally a
failure test has been added.
@srcarroll srcarroll deleted the flatten-elementwise-silenceable-failure branch June 5, 2024 02:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants