Page MenuHomePhabricator

[mlir][VectorOps][EDSC] Add EDSC for VectorOps
ClosedPublic

Authored by nicolasvasilache on Feb 8 2020, 10:52 AM.

Details

Summary

This revision adds EDSC support for VectorOps to enable the creation of a vector_matmul declaratively. The vector_matmul is a simple configuration
of the vector.contract op that follows the StructuredOps abstraction.

Diff Detail

Unit TestsFailed

TimeTest
10 msLLVM.Bindings/Go::Unknown Unit Message ("")
Script: -- : 'RUN: at line 1'; llvm-go test llvm.org/llvm/bindings/go/llvm

Event Timeline

Herald added a project: Restricted Project. · View Herald Transcript
nicolasvasilache marked 2 inline comments as done.Feb 8 2020, 11:00 AM
nicolasvasilache added inline comments.
mlir/include/mlir/IR/AffineMap.h
72

I am really not happy about this duplication but I tried a bunch of different things and could not get it down to one single form.
@rriddle @mehdi_amini suggestions most welcome!

mlir/lib/Dialect/VectorOps/EDSC/Builders.cpp
31

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()});
}
mehdi_amini accepted this revision.Feb 8 2020, 1:48 PM
mehdi_amini added inline comments.
mlir/include/mlir/IR/AffineMap.h
66

You wrote "Returns the AffineMap" but it returns a vector of AffineMap?

mlir/lib/Dialect/VectorOps/EDSC/Builders.cpp
31

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()

mlir/lib/IR/AffineMap.cpp
147

Using templates could avoid the temporary exprsVector, but I'm not sure it really matters...

nicolasvasilache marked 5 inline comments as done.Feb 10 2020, 11:58 AM
nicolasvasilache added inline comments.
mlir/lib/Dialect/VectorOps/EDSC/Builders.cpp
31

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.

mlir/lib/IR/AffineMap.cpp
147

Good enough, thanks!

nicolasvasilache marked 2 inline comments as done.

Address review comments.

Flushing out some answers.

This revision was not accepted when it landed; it landed in state Needs Review.Feb 10 2020, 12:09 PM
This revision was automatically updated to reflect the committed changes.
mehdi_amini added inline comments.Feb 10 2020, 12:10 PM
mlir/lib/Dialect/VectorOps/EDSC/Builders.cpp
31

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.