[PATCH] Add iterator for PHINode value/BB pair

[David Blaikie]

On Mon, Aug 3, 2015 at 10:38 AM, Pete Cooper <peter_cooper at apple.com> wrote:

> Sorry, just getting back to this
>

No worries


> since i’m starting to see other places where it could be useful.
>
> On Jul 23, 2015, at 1:07 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
>
> On Thu, Jul 23, 2015 at 12:29 PM, Pete Cooper <peter_cooper at apple.com>
> wrote:
>
>> Sorry this took so long.  Had to learn a whole bunch about varargs
>> templates.
>>
>> This is a first attempt at an LLVM zip iterator.  I needed to add methods
>> to STLExtras.h which do ++tuple<> and *tuple<>.  Originally I put those
>> under different names and didn’t override the operators, but then I didn’t
>> see any harm in overriding them.  I can rename them if anyone has a
>> preference for that.
>>
>
> Yeah, those do make me twitch a bit - I'm not sure if there's a more
> coherent argument against them or not.
>
> So i can totally understand the ‘twitch’.  Is it a bad twitch, or just
> something that takes a while to accept as how i’ve implemented it?
>

Uncertain twitch - a "this feels a bit not-good, but I'm not sure if it's
unacceptably so, or just a weird thing I could get used to". I wouldn't
mind roping in Mr. Smith or other C++ expert opinions (if you want to Dig
Doug out of the Swift Mines, etc).

If we can't find some more/better opinions it doesn't seem like it'd be too
costly to just avoid the quirk and implement a custom iterator that wraps
all the other iterators and has the operator overloads, etc.


>  I’d like to try get this to a state where we can consider committing it,
> so want to make sure everyone is happy with the solution.
>
>
> Oh, I see, this allows you to make the tuple itself an iterator. Yeah,
> also makes me twitch, but again - not sure if that's problematic or not.
> (I'm just imagining two libraries trying to pull the same game - and
> colliding in some painful way)
>
> (regardless of the name, wonder if the ++ and * could be generalized over
> each other (presumably the latter, which is more powerful (since it has a
> return value) than the former - but I don't know if tuple<void, void, ...>
> is valid... ;)))
>
> Yeah, i’ll try generalizing these over one another.  They also have
> different solutions for how to walk the tuple which isn’t ideal.  Better to
> just have a single way to apply an operator across a whole tuple.
>
>
>
>> The zip iterator itself is almost identical to the edge_iterator I had
>> before, just that it is now templated on its iterator type.
>>
>
> There seem to be some naming issues here... wait, no, I see it - the tuple
> is the iterator (retcon'd comments in above once I realized this)
>
>
>>  I’ve provided a subclass in zip_input_iterator for users who don’t need
>> all the parameters and just want to provide a varargs of iterator types.
>> All of this is of course up for discussion, this is just one way I could
>> see to implement it.
>>
>
> Probably could keep it in one class rather than zip_iterator and
> zip_input_iterator - just always pass the collection of iterator types and
> then deduce the tag from the lowest common denominator of the iterator tags
> of the specified iterators (not sure if there's already a trait/etc for
> joining those different things together and getting the least powerful
> iterator tag as a result). Save us having a taxonomy of zip_*_iterators.
>
> Nice idea to derive a common trait here.  I’ll give that a try.  That will
> make zip_iterator just take a T… as its only template parameter and remove
> the input_iterator.
>
>
>
>> I also allowed for zip_iterator_traits so that users could provide their
>> own type to return from operator*.  This is so that users don’t have to
>> remember which iterator is first, second, get<0>, etc.
>>
>
> Not sure if it's in 11 or 14, but there is type based access into tuple,
> get<Value*>, get<BasicBlock*>, etc - perhaps that's sufficient?
>
> Looks like its std::get<T>(tuple) and is in C++11, with more variety in
> C++14.  I think thats sufficient for many of the cases...
>
>
>
>>  Personally I think its much more readable than to have .Value, .Block,
>> etc.  This is optional though, as there’s a default implementation to
>> provide a dereferenced tuple type from operator*.
>>
>
> Interesting extension point, certainly. Could be problematic if there's
> the same pairing in two places with different meaning (int, int is
> size/length in one place, and something else elsewhere) and doesn't match
> up with different iterators (which might be weird to users - they change
> container type and then their code breaks because the trait is based on the
> iterator type, not the value type)
>
> Yeah, i think this is interesting, but you’re right to raise some
> questions here.  I think we would have to be careful where and when to
> expose this.  I’ll drop this extension for any initial commit, but perhaps
> if we need it later we can revisit it.
>
>
>
>> BTW, no idea how much of this is MSVC safe.  I did find a user of 'auto
>> fn()->decltype()’ in PDBTypes.h, so i’m not the first user of that, but
>> this will need someone with MSVC to ok the final implementation.
>>
>> I also need to write a unit test for all the functionality this enables.
>> Duncan mentioned std::copy requiring operator* to return a reference.
>>
>
> Interesting - I wonder how much/how it requires that? Because tuple<T&,
> U&> should be assignable (and assign to the reference members), etc, should
> work... even if the tuple is returned by value.
>
> Yeah, I think this is fine.  I just need to write a bunch of tests to
> double check, but as you’ve said, its a <T&, U&> tuple which is assignable.
>
>
>
>>  I’m still working through whether thats working or whether we only
>> really have foreach support right now.
>>
>> Duncan also pointed out in person, perhaps this is overkill as the
>> edge_iterator I had before was easier to understand than what we have
>> here.  Perhaps we’d prefer to hold off on a zip_iterator until we are sure
>> we have more users for it.
>>
>
> There are a fair few places where we iterate over two things in parallel
> and they're pretty awkward right now - could find a smattering of those and
> clean them up with the use of this tool (with a helper function to allow
> building these on-site within the range-for). I'm not sure of the easiest
> way to find places that could use this cleanup though... probably looking
> for for loops that either increment two things, or that use integers
> (because they're indexing into two containers).
>
> The case i’ve now found is in AArchISelLowering where we do:
>
> for (unsigned i = 0, e = ArgLocs.size(); i != e; ++i) {
>     CCValAssign &VA = ArgLocs[i];
>
>     if (Ins[i].Flags.isByVal()) {
>
> So we need to zip (ArgLocs, Ins).
>
> Given that this is done in pretty much every backend, I think there are
> going to be enough cases to justify adding zip_iterator as i’m sure we’ll
> find more over time (including the one Daniel B has)
>
> Will start trying to improve the patch based on all the feedback.
>
> Cheers,
> Pete
>
>
>
>>  If thats the case, then i’m happy for this to just be a demonstration of
>> how it could be done, but not necessarily something to commit now.
>>
>
> Fair, too.
>
> - Dave
>
>
>>
>> Cheers,
>> Pete
>>
>>
>>
>> On Jul 22, 2015, at 5:10 PM, David Blaikie <dblaikie at gmail.com> wrote:
>>
>> (oops, dropped the list by accident)
>> ---------- Forwarded message ----------
>> From: David Blaikie <dblaikie at gmail.com>
>> Date: Wed, Jul 22, 2015 at 5:10 PM
>> Subject: Re: [PATCH] Add iterator for PHINode value/BB pair
>> To: "Duncan P. N. Exon Smith" <dexonsmith at apple.com>
>>
>>
>>
>>
>> On Wed, Jul 22, 2015 at 5:05 PM, Duncan P. N. Exon Smith <
>> dexonsmith at apple.com> wrote:
>>
>>>
>>> > On 2015-Jul-22, at 16:57, David Blaikie <dblaikie at gmail.com> wrote:
>>> >
>>> >
>>> >
>>> > On Wed, Jul 22, 2015 at 4:50 PM, Duncan P. N. Exon Smith <
>>> dexonsmith at apple.com> wrote:
>>> >
>>> > > On 2015-Jul-22, at 15:07, David Blaikie <dblaikie at gmail.com> wrote:
>>> > >
>>> > >
>>> > >
>>> > > On Wed, Jul 22, 2015 at 12:11 PM, Duncan P. N. Exon Smith <
>>> dexonsmith at apple.com> wrote:
>>> > >
>>> > > > On 2015-Jul-21, at 21:20, Pete Cooper <peter_cooper at apple.com>
>>> wrote:
>>> > > >
>>> > > > Thanks for all the feedback.  This is a patch which addresses all
>>> of it.
>>> > > >
>>> > > > <phinode.diff>
>>> > > >
>>> > >
>>> > > > +
>>> > > > +    const PHINodeEdge operator*() const {
>>> > >
>>> > > No reason for the const in `const PHINodeEdge` here.
>>> > >
>>> > > to support operator-> you have to return a pointer, which means you
>>> need the PHINodeEdge storage inside the iterator to point to (& then you
>>> can just return a const ref from op*)
>>> >
>>> > Do we need operator->()?
>>> >
>>> > Seems poor form not to provide it (someone'll trip over it pretty
>>> quickly, I'd imagine)
>>> >
>>> > If so, we can return a proxy object:
>>> >
>>> >     struct PHINodeEdgeArrowProxy {
>>> >       const PHINodeEdge RefProxy;
>>> >     public:
>>> >       PHINodeEdgeArrowProxy(PHINodeEdge Edge) : RefProxy(Edge) {}
>>> >       const PHINodeEdge *operator->() const { return &RefProxy; }
>>> >     };
>>> >
>>> >     PHINodeEdgeArrowProxy operator->() const { return operator*(); }
>>> >
>>> > Then we avoid bloating the iterator, and only make the copy when we
>>> > actually need it.
>>> >
>>> > Non-conforming in terms of the iterator traits, I would imagine
>>>
>>> AFAICT, iterators only require that `i->m` has the same semantics as
>>> `(*i).m`.  The return type isn't specified.
>>>
>>
>> Fair enough - can't quite find the wording on what constitutes the
>> iterators value type (as is mentioned in the iterator traits) or how it
>> might relate, but the basic definition is as you've mentioned.
>>
>>
>>> > - but I take it that's the N1550 stuff you're talking about below? It
>>> makes these sort of proxy solutions valid?
>>> >
>>>
>>> IIRC, proxy solutions are always valid for InputIterator and for
>>> OutputIterator; it's ForwardIterator that prevents `operator*()`
>>> from returning a proxy.
>>>
>>
>> *nod* I don't mind violating that too much, if that's the preference.
>>
>>
>>> (This makes `std::vector<bool>::iterator` invalid.)
>>>
>>
>> Yep. Fails in so many ways.
>>
>>
>>>
>>> >
>>> > >
>>> > >
>>> > > > +      return { *Values, *Blocks };
>>> > > > +    }
>>> > > > +  };
>>> > > > +
>>> > >
>>> > > Otherwise LGTM.  Might want to pass it through clang-format; I
>>> noticed
>>> > > some minor whitespace oddities.
>>> > >
>>> > > > *snip*
>>> > > >> On Jul 21, 2015, at 8:21 PM, Duncan P. N. Exon Smith <
>>> dexonsmith at apple.com> wrote:
>>> > > >>
>>> > > >>
>>> > > >>
>>> > > >> If you drop the requirements from `forward_iterator` to
>>> > > >> `input_iterator`, then you're allowed to return a `PHINodeEdge`
>>> by-value
>>> > > >> here instead of by-reference (unfortunately this makes it illegal
>>> to use
>>> > > >> a bunch of STL algorithms; STL iterator traits are completely
>>> broken
>>> > > >> IMO).
>>> > > > I’m fine with this.  David, Chandler, please let me know how you
>>> feel about this.
>>> > > >
>>> > > > Also, i forgot to say that I considered doing a zip iterator and
>>> inheriting this from it.  This is something I think Chandler or David
>>> mentioned a few months ago.  If there’s been any progress in the C++
>>> committee on that then i’m happy to try implement something better.  If
>>> not, then i don’t think what I have here should be difficult to change in
>>> future.
>>> > >
>>> > > I guess a generalized version would return a
>>> > > `std::tuple<Value *const &, BasicBlock *const &>` or some such.  Not
>>> > > sure how to actually make zip iterators work well without something
>>> > > like N1550 though.
>>> > >
>>> > > What's N1550 offer to make zip work?
>>> >
>>> > s/work/& well/
>>> >
>>> > I think bloated iterators are bad, but without bloating them (so you
>>> > can return a T&), you can't call a zip_iterator a ForwardIterator,
>>> > which means you can't use it in a bunch of algorithms that you might
>>> > want to (such as the destination for `std::copy()`).
>>> >
>>> > N1550 let's you correctly identify the type of traversal the
>>> > zip_iterator can do, without requiring a T& return from operator*().
>>> >
>>> > > I wouldn't mind a slightly half-hearted version that works for
>>> basic/common cases...
>>>
>>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>
>>
>>
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150803/81e0e045/attachment.html>