-
Notifications
You must be signed in to change notification settings - Fork 72
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
dialects: (Transform) Add new selecting operations #3015
dialects: (Transform) Add new selecting operations #3015
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3015 +/- ##
==========================================
- Coverage 89.84% 89.81% -0.04%
==========================================
Files 411 411
Lines 51641 51892 +251
Branches 8027 8071 +44
==========================================
+ Hits 46399 46608 +209
- Misses 3961 3986 +25
- Partials 1281 1298 +17 ☔ View full report in Codecov by Sentry. |
"transform.named_sequence"() <{arg_attrs = [{transform.readonly}], function_type = (!transform.any_op) -> (), sym_name = "foo"}> ({ | ||
^bb0(%arg0: !transform.any_op): | ||
"transform.yield"() : () -> () | ||
}) : () -> () |
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.
Please use custom syntax in tests when possible, it makes them more readable
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 don't really see where custom syntax can be used in this case. are there any particular operations you're referring to?
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 don't have time for a super thorough review, but this looks very nice! @jorendumoulin ?
tests/filecheck/mlir-conversion/with-mlir/dialects/transform/transform_types.mlir
Outdated
Show resolved
Hide resolved
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.
this is looking good!
This is overall looking good, there are just small changes remaining. Could I just ask you to please make smaller pull requests in the future? A file that needs to be expanded explicitly in the GitHub UI is usually a signal to split up the changes into smaller diffs. For example, for this we could easily have merged the new attributes first, then the new operations. |
In this PR, 13 new core operations get added to the transform dialect. All of them useful to find and select the correct operation in a transformation. They are:
GetConsumerOfResult, GetDefiningOp, GetParentOp, GetProducerOfOperand, GetResultOp, GetTypeOp, IncludeOp, MatchOperationEmptyOp, MatchOperationNameOp, MatchParamCmpIOp, MergeHandlesOp, ParamConstantOp, SplitHandleOp,
All operations are tested using filecheck and pytest