[PATCH] D113511: [mlir] Optimize usage of llvm::mapped_iterator

[River Riddle via Phabricator via llvm-commits]

rriddle added inline comments.


================
Comment at: llvm/include/llvm/ADT/STLExtras.h:310-312
+/// A base type of mapped iterator, that is useful for building derived
+/// iterators that do not need/want to store the map function (as in
+/// mapped_iterator). These iterators must simply provide a `mapElement` method
----------------
dexonsmith wrote:
> rriddle wrote:
> > dblaikie wrote:
> > > Could this be implemented in mapped_iterator as an empty optimization if the mapping function happens to be stateless?
> > I think we can make it work for lambda callable types that capture no state.  For non-lambdas I'm not sure though, given that we don't have a reference to the callable, just its type.
> Maybe this (mapped_iterator_base) could be moved to a base class of mapped_iterator, where the latter adds the `getFunction()` API, implements `mapElement()`, and `friend`s mapped_iterator_base to give it access. This would keep the two implementations in sync.
> 
> For the EBO, `mapped_iterator` could delegate `getFunction()` to a new type `mapped_iterator_storage`, which avoids storing the function when possible.
Let me try to clean this up, thanks.


================
Comment at: llvm/include/llvm/ADT/STLExtras.h:315
+/// type.
+template <typename DerivedT, typename ItTy, typename ReferenceTy>
+class mapped_iterator_base
----------------
dexonsmith wrote:
> It'd be nice to avoid requiring `ReferenceTy`, using the same declval/decltype trick as above.
I'm not sure we can here, given that we don't have the exact function type, and I don't think we can resolve `DerivedT::mapElement` at this point? I could be wrong here.


================
Comment at: llvm/unittests/ADT/MappedIteratorTest.cpp:54
+                                          std::vector<int>::iterator, int> {
+    using BaseT::BaseT;
+
----------------
dexonsmith wrote:
> Seems like this would be more clearly written as:
> ```
> lang=c++
> using BaseT = CustomMapIterator::mapped_iterator_base;
> ```
> Why expose `BaseT`?
This is inheriting the constructor of the base class.

> Why expose BaseT?
I've gotten into a habit of using BaseT to avoid the need to have long constructor  inheritance lines, though that probably isn't necessary here given that you've pointed out that we don't need the super long template params (thanks for the comment on the other git commit BTW, need to clean other uses of this in MLIR. I wasn't sure if that was allowed on all of the compilers we support).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113511