[PATCH] D79489: [mlir][Linalg] NFC - Refactor and simplify Promotion

[Nicolas Vasilache via Phabricator via llvm-commits]

nicolasvasilache added inline comments.


================
Comment at: mlir/include/mlir/Dialect/Linalg/Transforms/Transforms.h:101
+  /// Allow the use of dynamicaly-sized buffers.
+  bool dynamicBuffers = false;
+  LinalgPromotionOptions &setDynamicBuffers(unsigned dynamic) {
----------------
mravishankar wrote:
> 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).
yes but in a future revision please, this wants to remain NFC.


================
Comment at: mlir/include/mlir/Dialect/Linalg/Transforms/Transforms.h:107
+  /// Alignment of promoted buffer. If `None` do not specify alignment.
+  Optional<unsigned> alignment = None;
+  LinalgPromotionOptions &setAlignment(unsigned align) {
----------------
mravishankar wrote:
> 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
yes but in a future revision please, this wants to remain NFC.
Feel free to rebase a memory space change on top of this.
I expect a few followup revisions will add a few more per operand options.


================
Comment at: mlir/include/mlir/Dialect/Linalg/Transforms/Transforms.h:267
+/// See `promoteSubViews` for more details.
 struct LinalgBasePromotionPattern : public RewritePattern {
   LinalgBasePromotionPattern(StringRef opName, MLIRContext *context,
----------------
mravishankar wrote:
> mravishankar wrote:
> > 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.
> Actually I see now how this is supposed to work. Disregard.
Ack


================
Comment at: mlir/lib/Dialect/Linalg/Transforms/Promotion.cpp:55
+  /// Allow the use of dynamicaly-sized buffers.
+  bool dynamicBuffers;
+  /// Alignment of promoted buffer.
----------------
mravishankar wrote:
> Same here. Maybe allow specification of dynamicBuffers, alignment and memory space per subview that is being promoted?
yes but in a future revision please, this wants to remain NFC.


================
Comment at: mlir/lib/Dialect/Linalg/Transforms/Promotion.cpp:102
+                         OperationFolder *folder,
+                         Optional<unsigned> alignment = None) {
   auto *ctx = size.getContext();
----------------
mravishankar wrote:
> Please add extra option to set memory space as well.
yes but in a future revision please, this wants to remain NFC.


================
Comment at: mlir/lib/Dialect/Linalg/Transforms/Promotion.cpp:208
     // boundary.
     linalg_fill(info->second.fullLocalView, fillVal);
   }
----------------
mravishankar wrote:
> 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).
> Same is true for the linalg_copy below.
I am envisioning is an analysis would place markers that will trigger patterns.
Anything unmarked would not be rewritten in the first place.

I think we can experiment with different scenarios in the future.
In any case, changing this now would make the revision non-NFC.


================
Comment at: mlir/lib/Dialect/Linalg/Transforms/Promotion.cpp:257
+                                  op.getOutputBuffers().end());
+  op.getOperation()->setOperands(0, opViews.size(), opViews);
 
----------------
ftynse wrote:
> If you want to call this function from a pattern, you are not allowed to do this (unless it's the root operation of the pattern and you surround it with a proper transaction start/stop)
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?


================
Comment at: mlir/lib/Dialect/Linalg/Transforms/Promotion.cpp:312
+    getFunction().walk([this, &folder](LinalgOp op) {
+      auto options = LinalgPromotionOptions().setDynamicBuffers(dynamicBuffers);
+      if (failed(promoteSubviewsPrecondition(op, options)))
----------------
mravishankar wrote:
> Why not just use the pattern above here instead of doing a walk explicitly?
Because it is unfortunately not strictly equivalent ..
The folder that is passed here does additional cleanups on the fly that cannot be applied as a pattern.
This is one of the reasons that motivates rewriting transforms as staged pattern applications (a few revisions down the line).

Once those are in we can drop the folder and use the patterns.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79489/new/

https://reviews.llvm.org/D79489