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

[River Riddle via Phabricator via llvm-commits]

rriddle added a comment.

In D113511#3119830 <https://reviews.llvm.org/D113511#3119830>, @lattner wrote:

> Whoa, thank you for tackling this!  The trick with `getFunction()` is interesting, I hadn't thought about that.
>
> I'm curious: did you consider splitting this into a new `static_mapped_iterator` class instead of keeping the mapped_iterator design of having a stored function?  The static_mapped_iterator could be implemented in roughly the same way, but instead of returning a closure, it could just invoke the "getFunction" member as a static member.  This would eliminate the lambdas and the function_ref in ResultElementTypeIterator.

Logically in mind I treated `mapped_iterator_base` as the base for all "map" iterators, and the current "mapped_iterator" is just a specialization of that which uses a stored callable. I opted against using `static` given that some of the iterators use state in the callable, e.g. the FloatElementIterator uses the float semantics field on the iterator, but maybe static still makes sense given that the callable itself is static?

> I'm not sure if that makes sense.  I'm imagining that clients would look like this:
>
>   template <typename UseIteratorT, typename OperandType>
>   class ValueUserIterator final :  public llvm::static_mapped_iterator<...
>   public:
>   
>     /// Return the map function used by this iterator.
>     static Operation *mapElement(OperandType &value) {
>         return value.getOwner();
>     }
>
> This would reduce pressure on the optimizer (good for -O0 builds in particular) and force a cleaner model.
>
> WDYT?

Yeah, I considered that but had some downstream users that wanted to interact with the mapping function directly.
I'll try to rewrite those to do something a bit different, let me update this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113511