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

[Nicolas Vasilache via Phabricator via llvm-commits]

nicolasvasilache marked 8 inline comments as done.
nicolasvasilache added a comment.
Herald added a subscriber: lucyrfox.

@yangjunpro addressed in https://reviews.llvm.org/D72112 thanks!



================
Comment at: mlir/include/mlir/Dialect/Linalg/IR/LinalgTraits.h:113
+           llvm::all_of(getInputsAndOutputs(),
+                        [](Value v) { return v.getType().isa<MemRefType>(); });
+  }
----------------
yangjunpro wrote:
> "Value v",  might change to pass by reference for better performance? Although it might be a little bit marginal here. 
Value is a POD, taking a reference would actually add a level of indirection and could be detrimental to perf.


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


================
Comment at: mlir/lib/Dialect/Linalg/IR/LinalgOps.cpp:302
+             << index << " must be " << outOpTy << ", but got " << resTy;
+    ++index;
+  }
----------------
yangjunpro wrote:
> 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. 
`getResultTypes` is an iterator range and does not work with random access.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72022