-
Notifications
You must be signed in to change notification settings - Fork 15.5k
[MLIR][NFC] Fix the pass description to describe what it actually does. #172306
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
base: main
Are you sure you want to change the base?
Conversation
|
@llvm/pr-subscribers-mlir-linalg @llvm/pr-subscribers-mlir Author: Petr Kurapov (kurapov-peter) ChangesI was trying out things and spent some time trying to fold a broadcast with pass only to find out that the declared functionality doesn't exist. Hope this saves someone's time. Full diff: https://github.com/llvm/llvm-project/pull/172306.diff 1 Files Affected:
diff --git a/mlir/include/mlir/Dialect/Linalg/Passes.td b/mlir/include/mlir/Dialect/Linalg/Passes.td
index 44da2965e6892..e1ea52ea2c21c 100644
--- a/mlir/include/mlir/Dialect/Linalg/Passes.td
+++ b/mlir/include/mlir/Dialect/Linalg/Passes.td
@@ -167,7 +167,7 @@ def LinalgInlineScalarOperandsPass : Pass<"linalg-inline-scalar-operands"> {
}
def LinalgFoldIntoElementwisePass : Pass<"linalg-fold-into-elementwise"> {
- let summary = "Fold transform, broadcast and other ops into elementwise";
+ let summary = "Fold transform ops into elementwise";
let dependentDialects = ["linalg::LinalgDialect"];
}
|
|
|
||
| def LinalgFoldIntoElementwisePass : Pass<"linalg-fold-into-elementwise"> { | ||
| let summary = "Fold transform, broadcast and other ops into elementwise"; | ||
| let summary = "Fold transform ops into elementwise"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you're there, can you actually fill the let description = [{ ... }]; field so that we have a more comprehensive documentation about what the pass does?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a short description. Not sure if my wording is clear enough. Let me know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel the description should be a bit more abstract.
Now it's too implementation specific.
|
Good call on clearing up the documentation here! Just thought to cross-link the following as it seems relevant: #167626 |
I was trying out things and spent some time trying to fold a broadcast with pass only to find out that the declared functionality doesn't exist. Hope this saves someone's time.