[mlir][linalg] Use hasPureTensorSemantics in TransposeMatmul methods. #146438
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The issue is triggered by llvm@ee070d0 that checks `TensorLikeType` when downstream projects use the pattern without registering bufferization::BufferizationDialect. The registeration is needed because the interface implementation for builtin types locate at `BufferizationDialect::initialize()`. However, we do not need to fix it by the registeration. The proper fix is using the linalg method, i.e., hasPureTensorSemantics. Signed-off-by: hanhanW <[email protected]>
hanhanW
requested review from
dcaballe,
nicolasvasilache and
rengolin
as code owners
|
cc @andrey-golubev (I can't add you as a reviewer.) |
|
@llvm/pr-subscribers-mlir-linalg @llvm/pr-subscribers-mlir Author: Han-Chung Wang (hanhanW) ChangesThe issue is triggered by ee070d0 that checks Full diff: https://github.com/llvm/llvm-project/pull/146438.diff 1 Files Affected:
diff --git a/mlir/lib/Dialect/Linalg/Transforms/TransposeMatmul.cpp b/mlir/lib/Dialect/Linalg/Transforms/TransposeMatmul.cpp
index e624f589917d1..233f180e48be0 100644
--- a/mlir/lib/Dialect/Linalg/Transforms/TransposeMatmul.cpp
+++ b/mlir/lib/Dialect/Linalg/Transforms/TransposeMatmul.cpp
@@ -38,7 +38,7 @@ FailureOr<Operation *> mlir::linalg::transposeMatmul(RewriterBase &rewriter,
matmulOp, "only matmul ops with non-extended semantics are supported");
}
- if (!bufferization::hasTensorSemantics(matmulOp))
+ if (!matmulOp.hasPureTensorSemantics())
return rewriter.notifyMatchFailure(
matmulOp, "only matmul ops with tensors are supported");
@@ -93,7 +93,7 @@ mlir::linalg::transposeBatchMatmul(RewriterBase &rewriter,
batchMatmulOp, "ops with user-defined maps are not supported");
}
- if (!bufferization::hasTensorSemantics(batchMatmulOp))
+ if (!batchMatmulOp.hasPureTensorSemantics())
return rewriter.notifyMatchFailure(
batchMatmulOp, "only matmul ops with tensors are supported");
|
|
The functionality is well tested in transpose-matmul.mlir. I'm not able to reproduce it using the transform dialect. My guess is that the interpreter pass already loads all the upstream dialects. I can only reproduce it with writing a new c++ pass. If it is a requirement, I can do so. But my feeling is that people will delete it in the future. |
There was a problem hiding this comment.
LGTM (I wonder how I didn't catch this when introducing a patch...). Could you add a test for this case so that the regression is covered?
Edit: saw your note regarding the reproduction in a different-scope scenario. Up to you then whether you want a test or not. I guess we could have some precautionary note saying "don't delete this test as it tests a different setup".
|
Thanks for the feedback! I decide to go without adding new tests because it is a simple and reasonable patch. Adding a c++ pass with different setup seems not worth it. I'll add the reason of why no additional tests are added in the PR description. |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/55/builds/13596 Here is the relevant piece of the build log for the reference |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The issue is triggered by ee070d0 that checks
TensorLikeTypewhen downstream projects use the pattern without registering bufferization::BufferizationDialect. The registration is needed because the interface implementation for builtin types locate atBufferizationDialect::initialize(). However, we do not need to fix it by the registration. The proper fix is using the linalg method, i.e., hasPureTensorSemantics.No additional tests are added because the functionality is well tested in transpose-matmul.mlir. To reproduce the issue, it requires a different setup, e.g., writing a new C++ pass, which seems not worth it.