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

[River Riddle via Phabricator via llvm-commits]

rriddle accepted this revision.
rriddle added inline comments.


================
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>();
----------------
rriddle wrote:
> getInputs().getTypes()
> 
> Also, use Type|Value instead of auto. They are practically the same amount of characters.
Can we use Type instead of auto here? Same below.


================
Comment at: mlir/tools/mlir-tblgen/RewriterGen.cpp:577
+      if (resultTree.isNativeCodeCall()) {
+        os << symbolInfoMap.getAllRangeUse(
+            val, "SmallVector<Value, 4> values{0};", "\n");
----------------
nicolasvasilache wrote:
> nicolasvasilache wrote:
> > rriddle wrote:
> > > nicolasvasilache wrote:
> > > > rriddle wrote:
> > > > > It isn't immediately clear to me why this is necessary. What return value is convertible to small vector, but not ArrayRef for example?
> > > > Reworked and added a comment.
> > > Could you use ValueRange instead?
> > Not immediately:
> > 
> > ```
> > /usr/local/google/home/ntv/work/github/llvm-project/mlir/include/mlir/IR/OperationSupport.h:623:3: note: candidate constructor
> >   ValueRange(iterator_range<OperandRange::iterator> values)
> >   ^
> > /usr/local/google/home/ntv/work/github/llvm-project/mlir/include/mlir/IR/OperationSupport.h:625:3: note: candidate constructor
> >   ValueRange(iterator_range<ResultRange::iterator> values)
> >   ^
> > /usr/local/google/home/ntv/work/github/llvm-project/mlir/include/mlir/IR/OperationSupport.h:627:3: note: candidate constructor
> >   ValueRange(ArrayRef<Value> values = llvm::None);
> >   ^
> > /usr/local/google/home/ntv/work/github/llvm-project/mlir/include/mlir/IR/OperationSupport.h:628:3: note: candidate constructor
> >   ValueRange(OperandRange values);
> >   ^
> > /usr/local/google/home/ntv/work/github/llvm-project/mlir/include/mlir/IR/OperationSupport.h:629:3: note: candidate constructor
> >   ValueRange(ResultRange values);
> >   ^
> > /usr/local/google/home/ntv/work/github/llvm-project/mlir/include/mlir/Support/STLExtras.h:223:3: note: candidate inherited constructor
> >   indexed_accessor_range_base(const iterator_range<iterator> &range)
> >   ^
> > /usr/local/google/home/ntv/work/github/llvm-project/mlir/include/mlir/IR/OperationSupport.h:613:21: note: constructor from base class 'indexed_accessor_range_base<mlir::ValueRange, llvm::PointerUnion<const mlir::Value *, mlir::OpOperand *, mlir::OpResult *>, mlir::Value, mlir::Value, mlir::Value>' inherited here
> >   using RangeBaseT::RangeBaseT;
> > 
> > ```
> Forgot the first line, here:
> ```
> tools/mlir/test/lib/Transforms/../DeclarativeTransforms/TestLinalgTransformPatterns.h.inc:579:19: error: call to constructor of 'mlir::ValueRange' is ambiguous
>     for (auto v : ValueRange{ {permuteGenericLinalgOp(rewriter, op, {1, 2, 0}, "PERMUTE")} }) { tblgen_repl_values.push_back(v); }
>                   ^         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ```
This is extremely unfortunate and should really get some attention if we have to do this. Can you file a bug so that this is cleaned up? We shouldn't be materializing vectors like this if we don't need to.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72022