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

[Mahesh Ravishankar via Phabricator via llvm-commits]

mravishankar requested changes to this revision.
mravishankar added a comment.
This revision now requires changes to proceed.

Some initial 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) {
----------------
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).


================
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) {
----------------
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


================
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,
----------------
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.


================
Comment at: mlir/lib/Dialect/Linalg/Transforms/Promotion.cpp:55
+  /// Allow the use of dynamicaly-sized buffers.
+  bool dynamicBuffers;
+  /// Alignment of promoted buffer.
----------------
Same here. Maybe allow specification of dynamicBuffers, alignment and memory space per subview that is being promoted?


================
Comment at: mlir/lib/Dialect/Linalg/Transforms/Promotion.cpp:98
 
+/// Alloc a new buffer of `size`. If `dynamicBuffers` is true allocate exactly
+/// the size needed, otherwise try to allocate a static bounding box.
----------------
Thanks for the comment. Makes it clear what dynamicBuffers are for.


================
Comment at: mlir/lib/Dialect/Linalg/Transforms/Promotion.cpp:102
+                         OperationFolder *folder,
+                         Optional<unsigned> alignment = None) {
   auto *ctx = size.getContext();
----------------
Please add extra option to set memory space as well.


================
Comment at: mlir/lib/Dialect/Linalg/Transforms/Promotion.cpp:283
     auto sv = isa_and_nonnull<SubViewOp>(en.value().getDefiningOp());
-    if (sv && (!operandIndicesToPromote.hasValue() ||
-               operandIndicesToPromote->count(en.index())))
-      return success();
+    if (sv)
+      if (!options.operandsToPromote.hasValue() ||
----------------
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)


================
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)))
----------------
Why not just use the pattern above here instead of doing a walk explicitly?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79489