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

[River Riddle via Phabricator via llvm-commits]

rriddle requested changes to this revision.
rriddle added inline comments.
This revision now requires changes to proceed.


================
Comment at: mlir/include/mlir/Dialect/Linalg/IR/LinalgTraits.h:118
+    SmallVector<RankedTensorType, 4> res;
+    for (auto v : getInputs()) {
+      auto t = v.getType().template dyn_cast<RankedTensorType>();
----------------
getInputs().getTypes()

Also, use Type|Value instead of auto. They are practically the same amount of characters.


================
Comment at: mlir/include/mlir/Dialect/Linalg/IR/LinalgTraits.h:119
+    for (auto v : getInputs()) {
+      auto t = v.getType().template dyn_cast<RankedTensorType>();
+      if (t)
----------------
nit: Merge assignment and condition
if (auto t = ...)


================
Comment at: mlir/include/mlir/Dialect/Linalg/IR/LinalgTraits.h:128
+    SmallVector<RankedTensorType, 4> res;
+    for (auto v : getOutputs()) {
+      auto t = v.getType().template dyn_cast<RankedTensorType>();
----------------
Same comments as above.


================
Comment at: mlir/include/mlir/Dialect/Linalg/Transforms/LinalgTransforms.h:83
+LogicalResult vectorizeGenericLinalgOpPrecondition(Operation *op);
+SmallVector<Value, 0> vectorizeGenericLinalgOp(PatternRewriter &rewriter,
+                                               Operation *op);
----------------
Why SmallVector<..., 0>?


================
Comment at: mlir/lib/Dialect/Linalg/IR/LinalgOps.cpp:67
+  if (!outputTensorTypes.empty())
+    interleaveComma(outputTensorTypes, p << " -> ");
 }
----------------
Can you just use the same mechanism as 2 lines above? i.e. just stream it.


================
Comment at: mlir/lib/Dialect/Linalg/IR/LinalgOps.cpp:102
+  SmallVector<Type, 8> tensorResultTypes;
+  parser.parseOptionalArrowTypeList(tensorResultTypes);
+  if (!tensorResultTypes.empty())
----------------
You need to check this for failure


================
Comment at: mlir/tools/mlir-tblgen/RewriterGen.cpp:577
+      if (resultTree.isNativeCodeCall()) {
+        os << symbolInfoMap.getAllRangeUse(
+            val, "SmallVector<Value, 4> values{0};", "\n");
----------------
It isn't immediately clear to me why this is necessary. What return value is convertible to small vector, but not ArrayRef for example?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72022