This revision introduces LinalgPromotionOptions to more easily control the application of promotion patterns. It also simplifies the different entry points into Promotion in preparation for some behavior change in subsequent revisions.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Some initial comments.
mlir/include/mlir/Dialect/Linalg/Transforms/Transforms.h | ||
---|---|---|
101 | Should this be a a per operand specification? Suggestion is to have a separate struct that captures per operand specification for this, alignment (and memory space as well. See below). | |
107 | Could we add an option to set memory space here as well? I was touching the same file in a WIP change of mine to add memory space. But you are touching these anyway | |
267 | This seems to diverge from the way patterns are exposed in other places. The convention is AFAIK to use "populate.....Pattern" which takes a OwningRewritePatternList as an argument (by reference) and inserts the pattern. With that you dont need to have the pattern itself exposed just an API to insert the pattern. You could then add as many patterns as you want into that. | |
mlir/lib/Dialect/Linalg/Transforms/Promotion.cpp | ||
55 | Same here. Maybe allow specification of dynamicBuffers, alignment and memory space per subview that is being promoted? | |
98 | Thanks for the comment. Makes it clear what dynamicBuffers are for. | |
102 | Please add extra option to set memory space as well. | |
283–286 | Please add braces around multi-line statements (disclaimer : I dont know if thats the coding standard for LLVM, but dropping trivial braces for single line body seems fine, but this is not a single line body) | |
312 | Why not just use the pattern above here instead of doing a walk explicitly? |
mlir/include/mlir/Dialect/Linalg/Transforms/Transforms.h | ||
---|---|---|
267 | Actually I see now how this is supposed to work. Disregard. | |
mlir/lib/Dialect/Linalg/Transforms/Promotion.cpp | ||
208 | I think these fill needs to have the marker added to it too. If not they can be intercepted by the pattern rewriter which would try to apply transformations on them (AFAIK the marker is supposed to prevent that). |
mlir/include/mlir/Dialect/Linalg/Transforms/Transforms.h | ||
---|---|---|
101 | yes but in a future revision please, this wants to remain NFC. | |
107 | yes but in a future revision please, this wants to remain NFC. | |
267 | Ack | |
mlir/lib/Dialect/Linalg/Transforms/Promotion.cpp | ||
55 | yes but in a future revision please, this wants to remain NFC. | |
102 | yes but in a future revision please, this wants to remain NFC. | |
208 | I am envisioning is an analysis would place markers that will trigger patterns. I think we can experiment with different scenarios in the future. | |
257 | This is indeed called from under rewriter.updateRootInPlace(op, [&]() { and the prereq checks that it will succeed. I can revert to the old behavior if you prefer but it seems like a legitimate use to me? | |
312 | Because it is unfortunately not strictly equivalent .. Once those are in we can drop the folder and use the patterns. |
Could you let me know when you are done with most refactoring so that I can pick up some other things that intersected with this refactoring work. I dont want to be touching the same code will its being modified.
mlir/lib/Dialect/Linalg/Transforms/Promotion.cpp | ||
---|---|---|
208 | Ok, lets sync about this offline. Maybe the way I was thinking about this is reverse from what you mentioned. By reading the code, I was thinking that you do a transformation and set a marker, preventing it from being transformed again. But what is being done is that it is setting the maker for the next stage of transformation to kick in. I need to account for this in IREE codegen. Will take a look after all the refactoring is done. |
mlir/include/mlir/Dialect/Linalg/Transforms/Transforms.h | ||
---|---|---|
96 | @nicolasvasilache ping? |
operandsToPromote = DenseSet<unsigned>(operands.begin(), operands.end())?