[PATCH] D78996: [MLIR][LINALG] Convert Linalg to Linalg

[Alex Zinenko via Phabricator via llvm-commits]

ftynse added a comment.

Here's a more detailed review. I am not familiar enough with the buffer assignment mechanism that was introduced, deferring that to @pifon2a



================
Comment at: mlir/include/mlir/Dialect/Linalg/Passes.h:20
 namespace mlir {
+class BufferAssignmentPlacer;
 class FuncOp;
----------------
These forward declarations look unnecessary in this file


================
Comment at: mlir/lib/Dialect/Linalg/Transforms/TensorsToBuffers.cpp:44
+    SmallVector<Value, 2> newBuffers;
+    for (auto result : results) {
+      auto type = result.getType().cast<ShapedType>();
----------------
Please, call `.reserve` on vectors before using `push_back` in a loop


================
Comment at: mlir/lib/Dialect/Linalg/Transforms/TensorsToBuffers.cpp:47
+      if (!type)
+        op.emitOpError()
+            << "tensor to buffer conversion expects ranked results";
----------------
rewriter.notifyMatchFailure

If this is an invariant of the op itself, it should be checked in the verifier and can be asserted to be correct here.


================
Comment at: mlir/lib/Dialect/Linalg/Transforms/TensorsToBuffers.cpp:70
+    // Move regions from the old operation to the new one.
+    auto &region = linalgOp.region();
+    rewriter.inlineRegionBefore(op.region(), region, region.end());
----------------
Nit: we prefer `auto` in places where it improves readability, i.e. if the type is too long to spell out (e.g., iterators, zippers) or if it is obvious due to a `cast`. I think `Region &` would be just fine here.


================
Comment at: mlir/lib/Dialect/Linalg/Transforms/TensorsToBuffers.cpp:77
+      auto type = result.getType().cast<ShapedType>();
+      entryBlock.addArgument(type.getElementType());
+    }
----------------
.addArgument (and any inplace modification) is not allowed in a conversion pattern because it's not undoable. You'll have to create a new block with `rewriter.createBlock` and move/clone the operations from the existing block.


================
Comment at: mlir/lib/Dialect/Linalg/Transforms/TensorsToBuffers.cpp:86
+/// tensors to buffers.
+void populateConvertLinalgOnTensorsToBuffersPattern(
+    MLIRContext *context, BufferAssignmentPlacer *placer,
----------------
LLVM prefers declaring functions `static` to putting them in an anonymous namespace, unlike classes.


================
Comment at: mlir/lib/Dialect/Linalg/Transforms/TensorsToBuffers.cpp:93-94
+                  GenericOpConverter,
+                  NonVoidToVoidReturnOpConverter<
+                      mlir::ReturnOp, mlir::ReturnOp, linalg::CopyOp>
+  >(context, placer, converter);
----------------
Could you have `using <foo> = NonVoidToVoidReturnOpConverter<mlir::ReturnOp, mlir::ReturnOp, linalg::CopyOp>` and put just `<foo>` here to get less crazy formatting? Don't know what's the name for `<foo>` though.


================
Comment at: mlir/lib/Dialect/Linalg/Transforms/TensorsToBuffers.cpp:102
+struct ConvertLinalgOnTensorsToBuffers
+    : mlir::PassWrapper<ConvertLinalgOnTensorsToBuffers,
+                        OperationPass<ModuleOp>> {
----------------
Given that the pass is declaratively specified, use `ConvertLinalgOnTensorsToBuffersPassBase<ConvertLinalgOnTensorToBuffers>` instead. (also feel free to choose a shorter name for the anonymous class)


================
Comment at: mlir/lib/Dialect/Linalg/Transforms/TensorsToBuffers.cpp:113
+        Optional<ConversionTarget::DynamicLegalityCallbackFn>(
+            [&](Operation *op) {
+              auto isIllegalType = [&](Type type) {
----------------
Style nit: let's put this lambda outside the function call


================
Comment at: mlir/lib/Dialect/Linalg/Transforms/TensorsToBuffers.cpp:122
+    // Mark return operations illegal as long as they return values.
+    target.addDynamicallyLegalOp<mlir::ReturnOp>(
+        [](mlir::ReturnOp returnOp) { return returnOp.getNumOperands() == 0; });
----------------
This looks like it would make impossible to have simple helper functions that have no Linalg in them.

```
func @square(%arg0: f32) {
  %0 = mulf %arg0, %arg0 : f32
  return %0 : f32
}

func @foo(...) {
  linalg.generic (...buffers...) {
    %0 = call @square(...) : (f32) -> f32
    linalg.yield %0 : f32
  }
}
```


================
Comment at: mlir/lib/Dialect/Linalg/Transforms/TensorsToBuffers.cpp:126
+    // Mark the function operation illegal as long as an argument is tensor.
+    target.addDynamicallyLegalOp<FuncOp>([&](FuncOp funcOp) {
+      return converter.isSignatureLegal(funcOp.getType());
----------------
Similar to the above, but I might understand that we want this pass to fail if there remains any function on tensors (maybe it doesn't use linalg at all...)


================
Comment at: mlir/test/Dialect/Linalg/tensors-to-buffers.mlir:15
+//      CHECK: (%[[NEW_ARG0:.*]]: [[TYPE:.*]], %[[ARG1_RESULT:.*]]: [[TYPE]], %[[ARG2_RESULT:.*]]: [[TYPE]])
+// CHECK-NEXT: %[[FIRST_ALLOC:.*]] = alloc() : [[TYPE]]
+// CHECK-NEXT: %[[SECOND_ALLOC:.*]] = alloc() : [[TYPE]]
----------------
Do we actually care about lines being strictly consecutive, or just them being ordered suffices? If the latter, drop the `-NEXT`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78996