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

[Jun Yang via Phabricator via llvm-commits]

yangjunpro added inline comments.


================
Comment at: mlir/include/mlir/Dialect/Linalg/IR/LinalgStructuredOps.td:550
+    Tensor values must be legalized by a buffer allocation pass before most
+    transformations can be applied. In particular, transformations that create
+    control flow around linalg.generic operations are not expected to mix with
----------------
"that create"==>"which create", small typos


================
Comment at: mlir/include/mlir/Dialect/Linalg/IR/LinalgTraits.h:113
+           llvm::all_of(getInputsAndOutputs(),
+                        [](Value v) { return v.getType().isa<MemRefType>(); });
+  }
----------------
"Value v",  might change to pass by reference for better performance? Although it might be a little bit marginal here. 


================
Comment at: mlir/include/mlir/Dialect/Linalg/IR/LinalgTraits.h:127
+    for (auto type : getOutputs().getTypes())
+      if (auto t = type.template dyn_cast<RankedTensorType>())
+        res.push_back(t);
----------------
There is a little bit duplicated code between getInputTensorTypes() and getOutputTensorTypes(), do we need to extract a small common function for reusability? 


================
Comment at: mlir/include/mlir/Dialect/Linalg/Transforms/LinalgTransforms.h:83
+LogicalResult vectorizeGenericLinalgOpPrecondition(Operation *op);
+SmallVector<Value, 0> vectorizeGenericLinalgOp(PatternRewriter &rewriter,
+                                               Operation *op);
----------------
Can't we directly write the return type as SmallVector<Value> ? "0" looks peculiar here. 


================
Comment at: mlir/lib/Dialect/Linalg/IR/LinalgOps.cpp:302
+             << index << " must be " << outOpTy << ", but got " << resTy;
+    ++index;
+  }
----------------
If we already have the index, why do we bother to use the zip()?
I think by directly use op.getResultTypes()[index], outputTensorTypes[index] is enough. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72022