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

[Nicolas Vasilache via Phabricator via llvm-commits]

nicolasvasilache marked 5 inline comments as done.
nicolasvasilache added inline comments.


================
Comment at: mlir/lib/Dialect/VectorOps/EDSC/Builders.cpp:30
+      IndexingExprs{A.getExprs(), B.getExprs(), C.getExprs()},
+      ArrayRef<StringRef>{functional::map(toString, iteratorTypes)});
+}
----------------
mehdi_amini wrote:
> 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()
That falls into the category of multiple attrs and having builders for multiple combinations of attrs vs underlying values.
Ideally I wouldn't fall in 2^n behavior here.
So either I'd go full attributes or full underlying values and reconstruct attributes, which is what I went for here.

Then orthogonally, the materialization is really a "proper API with ranges" issues on which I gave up in this revision.
If we were to care about optimization of this particular thing I'd prefer more effort into making ranges more usable.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74284