[PATCH] D27792: IfConversion: Use reverse_iterator to simplify. NFC

Tue Dec 20 17:55:20 PST 2016

> On 2016-Dec-20, at 15:44, Kyle Butt <iteratee at google.com> wrote:
> 
> Normally, reverse iterators are not off-by-one.  See the reverse iterators provided by containers like std::set, std::map, std::list, etc..  All of these have a handle to the same node that is getting dereferenced.  They work by having a sentinel, rend(), that's past the final (reverse) element.  They have similar guarantees to the forward iterators: they are only invalidated when the pointed-to-node gets deleted.
> 
> This isn't true. std::set, std::map, std::list also do the off by one accounting.

Huh... I stand corrected.  Not sure how I got that :/.  Regretfully, I had this incorrect impression when I was working on this, and probably convinced others of it.

I stand by my point that the new invalidation semantics are strictly better.  But being inconsistent is awkward...

> I haven't looked at this patch; from the limited context here, I don't understand why you'd *want* off-by-one accounting.  Off-by-one accounting is usually complicated and hard to reason about...
> 
> 
> When dealing with a range, off by one makes perfect sense. When you need to shrink from the front, you work from [begin, end)
> Then to start working from the back you flip the iterators and now you have (rend, rbegin]

It's the invalidation that's hard to reason about.

>  
> > On 2016-Dec-20, at 13:43, David Blaikie <dblaikie at gmail.com <mailto:dblaikie at gmail.com>> wrote:
> >
> > Duncan - looks like you were working on ilist when you added this comment about the reverse iterator - any particular insight that might be relevant here?
> >
> > Conversely, Kyle - is the comment needed? If getReverse gives a reverse iterator that points to the current element of the original iterator, it seems like that'd be obvious & not require a comment (& perhaps that was the point of the ilist API change there)? (If I'm understanding correctly)
> >
> > On Tue, Dec 20, 2016 at 1:01 PM Kyle Butt via Phabricator <reviews at reviews.llvm.org <mailto:reviews at reviews.llvm.org>> wrote:
> > iteratee added inline comments.
> >
> >
> > ================
> > Comment at: lib/CodeGen/IfConversion.cpp:646-647
> >    // Now, in preparation for counting duplicate instructions at the ends of the
> > -  // blocks, move the end iterators up past any branch instructions.
> > -  --TIE;
> > -  --FIE;
> > -
> > -  // After this point TIB and TIE define an inclusive range, which means that
> > -  // TIB == TIE is true when there is one more instruction to consider, not at
> > -  // the end. Because we may not be able to go before TIB, we need a flag to
> > -  // indicate a completely empty range.
> > -  bool TEmpty = false, FEmpty = false;
> > -
> > -  // Upon exit TIE and FIE will both point at the last non-shared instruction.
> > -  // They need to be moved forward to point past the last non-shared
> > -  // instruction if the range they delimit is non-empty.
> > -  auto IncrementEndIteratorsOnExit = make_scope_exit([&]() {
> > -    if (!TEmpty)
> > -      ++TIE;
> > -    if (!FEmpty)
> > -      ++FIE;
> > -  });
> > +  // blocks, switch to reverse_iterators. Note that these are NOT std
> > +  // reverse_iterators, so we have to manually do the off by one accounting.
> > +  MachineBasicBlock::reverse_iterator RTIE = ++(TIE.getReverse());
> > ----------------
> > dblaikie wrote:
> > > Is this something we should be relying on? Does the spec say how reverse iterators can/should be constructed from non-reverse iterators in general? (and is MBB violating that contract?)
> > >
> > > Should we be fixing MBB's reverse_iterators to do this accounting if it's the usual/expected/precedential (which is totally a word) thing?
> > See the comment in include/llvm/ADT/ilist_iterator.h:
> >
> >   /// Convert from an iterator to its reverse.
> >   ///
> >   /// TODO: Roll this into the implicit constructor once we're sure that no one
> >   /// is relying on the std::reverse_iterator off-by-one semantics.
> >
> > I take that to mean that we are intentionally not supplying the std::reverse_iterator semantics.
> >
> > The comment is aimed at a reader familiar with those semantics.
> >
> >
> > https://reviews.llvm.org/D27792 <https://reviews.llvm.org/D27792>
> >
> >
> >
> 
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20161220/89a79c61/attachment.html>