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

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini 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)});
+}
----------------
nicolasvasilache wrote:
> 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.
I'm not sure what you mean: your builder API is encouraging a more costly model: you materialize two SmallVector instead of one.
Just *not* adding your builder seems to both reduce the number of builder and increasing efficiency.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74284