[PATCH] D74284: [mlir][VectorOps][EDSC] Add EDSC for VectorOps

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini accepted this revision.
mehdi_amini added inline comments.


================
Comment at: mlir/include/mlir/IR/AffineMap.h:66
 
+  /// Returns the AffineMap with as many results as `exprs.size()`, as many dims
+  /// as the largest dim in `exprs` and as many symbols as the largest symbol in
----------------
You wrote "Returns the AffineMap" but it returns a vector of AffineMap?



================
Comment at: mlir/lib/Dialect/VectorOps/EDSC/Builders.cpp:30
+      IndexingExprs{A.getExprs(), B.getExprs(), C.getExprs()},
+      ArrayRef<StringRef>{functional::map(toString, iteratorTypes)});
+}
----------------
nicolasvasilache wrote:
> One may think it is not great to materialize here but after spending too much time, I got to the following and still got compilation errors with issues converting to `const StringRef *`.
> At this point I find it counterproductive and prefer the current form.
> 
> ```
> using IteratorTypeList = ArrayRef<IteratorType>;
> class iterator_value_iterator final
>     : llvm::mapped_iterator<IteratorTypeList::iterator,
>                             StringRef (*)(IteratorType)> {
> public:
>   explicit iterator_value_iterator(IteratorTypeList::iterator it)
>       : llvm::mapped_iterator<IteratorTypeList::iterator,
>                               StringRef (*)(IteratorType)>(
>             it, [](IteratorType iter) { return toString(iter); }) {}
>   StringRef operator*() const { return toString(*this->I); }
> };
>  
> llvm::iterator_range<iterator_value_iterator>
> makeRange(ArrayRef<IteratorType> iteratorTypes) {
>   return llvm::make_range(iterator_value_iterator(iteratorTypes.begin()),
>                           iterator_value_iterator(iteratorTypes.end()));
> }
>  
> Value mlir::edsc::ops::vector_contraction(
>     StructuredIndexed A, StructuredIndexed B, StructuredIndexed C,
>     ArrayRef<IteratorType> iteratorTypes) {
>   using IndexingExprs = ArrayRef<ArrayRef<AffineExpr>>;
>   auto range = makeRange(iteratorTypes);
>   return vector_contract(
>       A.getValue(), B.getValue(), C.getValue(),
>       IndexingExprs{A.getExprs(), B.getExprs(), C.getExprs()},
>       ArrayRef<StringRef>{range.begin(), range.end()});
> }
> ``` 
What about materializing directly as an Array of StringAttr here instead of doing it in the builder?
Right now you materialize here first and then the builder will trigger another call to functional::map()


================
Comment at: mlir/lib/IR/AffineMap.cpp:147
+  return inferFromExprList(exprsVector);
+}
+
----------------
Using templates could avoid the temporary `exprsVector`, but I'm not sure it really matters...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74284