[PATCH] D72863: [mlir][Linalg] Add tensor support to Linalg EDSC Builders

[Alex Zinenko via Phabricator via llvm-commits]

ftynse added a comment.

I'm not sure I understand what happens to the Op operands that are supposed to be tensors in individual `linalg_` constructors. The comment says `outputs` should not contain tensors, but in individual constructors there's no generic `outputs`, and some `StructuredIndexed` *must* be passed in.

Nit: typo in the commit description "identity identity"



================
Comment at: mlir/include/mlir/Dialect/Linalg/EDSC/Builders.h:186
+///
+/// This accepts both buffers and tensors as `inputs` but only buffers as
+/// `outputs`. Output tensors can be specified with `resultTensorTypes`, in
----------------
There are no arguments called `inputs` and `outputs` here and below. I can suggest the formulation "StructuredIndexed for inputs may wrap a buffer or a tensor, those for outputs may wrap only buffers". Same below.

I'd drop the "TODO", it's the same as above.


================
Comment at: mlir/include/mlir/Dialect/Linalg/EDSC/Builders.h:195-196
 Operation *linalg_pointwise(UnaryPointwiseOpBuilder unaryOp,
-                            StructuredIndexed I, StructuredIndexed O);
+                            StructuredIndexed I, StructuredIndexed O,
+                            ArrayRef<Type> resultTensorTypes = {});
 
----------------
It's not clear what happens to `O`, which I presume is the output, if you want tensor outputs.


================
Comment at: mlir/lib/Dialect/Linalg/EDSC/Builders.cpp:235
+                                       ArrayRef<Type> resultTensorTypes) {
   ;
   using edsc::intrinsics::tanh;
----------------
Let's clean this semicolon :)


================
Comment at: mlir/lib/Dialect/Linalg/EDSC/Builders.cpp:292
+
+  assert(!C.getType().isa<MemRefType>() || resultTensorTypes.empty());
+  StructuredIndexed allIndexed[3]{A({m, k}), B({k, n}), C({m, n})};
----------------
Please add a string with description to the assert


================
Comment at: mlir/lib/Dialect/Linalg/EDSC/Builders.cpp:293
+  assert(!C.getType().isa<MemRefType>() || resultTensorTypes.empty());
+  StructuredIndexed allIndexed[3]{A({m, k}), B({k, n}), C({m, n})};
+  ArrayRef<StructuredIndexed> inputs =
----------------
You can do something like

```
ArrayRef<StructuredIndexed> all(allIndexed)
bool isResultBuffer = C.getType().isa<MemRefType>();
auto inputs = all.drop_back(isResultBuffer);
auto outputs = all.take_back(isResultBuffer)
```

or even

```
ArrayRef<StructuredIndexed> all({A({m, k}), B({k, n}), C({m, n})});
```
```


================
Comment at: mlir/lib/Dialect/Linalg/EDSC/Builders.cpp:340
   // clang-format on
+  auto inputs = (O.getType().isa<MemRefType>())
+                    ? ArrayRef<StructuredIndexed>{allIndexed, allIndexed + 2}
----------------
Same as above


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72863