[PATCH] D72022: [mlir][Linalg] Extend generic ops to allow tensors

[Alex Zinenko via Phabricator via llvm-commits]

ftynse added inline comments.


================
Comment at: mlir/include/mlir/Dialect/Linalg/IR/LinalgStructuredOps.td:92
     InterfaceMethod<[{
-        Query the index of the given input value, or `None` if the value is not
-        an input.
+        Query the index of the given input value `v`, or `None` if the value is
+        not an input.
----------------
Nit: Query -> Return, otherwise it souds like you query none


================
Comment at: mlir/include/mlir/Dialect/Linalg/IR/LinalgStructuredOps.td:391
+  And<[
+    Or<[AnyRankedTensor.predicate, AnyStridedMemRef.predicate]>,
+    CPred<"$_self.cast<ShapedType>().getRank() == " # rank>]
----------------
Reuse `LinalgOperand.predicate` here


================
Comment at: mlir/include/mlir/Dialect/Linalg/IR/LinalgStructuredOps.td:541
+        tensor<?x?xf32>,
+        memref<?x?xf32, stride_specification>,
+        tensor<?x?xf32>
----------------
Do you allow mixing tensor and memref arguments? If so, consider mentioning it explicitly.


================
Comment at: mlir/include/mlir/Dialect/Linalg/IR/LinalgStructuredOps.td:602
         {
+          // Some optional condition on offset_m, offset_n and offset_k.
           %d = mulf %a, %b: f32
----------------
Is this a placeholder? Consider using an op instead "some.condition(...)", a comment looks like it's related to the following code, but it is not.


================
Comment at: mlir/lib/Dialect/Linalg/IR/LinalgOps.cpp:290
+  if (outputTensorTypes.size() != op.getNumResults())
+    return op.emitError("op expected #output tensor operands (")
+           << outputTensorTypes.size() << ") to match #results ("
----------------
Nit: use `emitOpError` if you want to refer the op name and drop the "op " part of the message


================
Comment at: mlir/lib/Dialect/Linalg/Transforms/LinalgTransforms.cpp:187
+
+  if (failed(vectorizeGenericLinalgOpPrecondition(op)))
+    llvm_unreachable("DRR failure case must be a precondition");
----------------
should this rather be an assert(succeeded(...) && ...) ?


================
Comment at: mlir/lib/Dialect/Linalg/Transforms/LinalgTransforms.cpp:266
+  if (llvm::none_of(linOp.getInputsAndOutputs(), [](Value v) {
+        return dyn_cast_or_null<SubViewOp>(v.getDefiningOp());
+      }))
----------------
return isa_and_nonnull<...


================
Comment at: mlir/lib/Dialect/VectorOps/VectorTransforms.cpp:500
   // Unroll 'op' with 'iterationBounds' to 'targetShape'.
-  return unrollSingleResultStructuredOp(op, iterationBounds, vectors,
-                                        resultIndex, targetShape, builder);
+  return SmallVector<Value, 1>{unrollSingleResultStructuredOp(
+      op, iterationBounds, vectors, resultIndex, targetShape, builder)};
----------------
Nit: would llvm::to_vector<1> work?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72022