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

[Duncan P. N. Exon Smith via llvm-commits]

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.

std::reverse_iterator is an adaptor meant for "abnormal" situations.  Array containers (like std::vector) can't implement the "normal" semantics, since you can't have a handle to something past the final-reverse/first-forward element of the array.  In array containers, iterators are always invalidated by operations that mutate the container.

Before my commits, std::reverse_iterator was being used to implement ilist::reverse_iterator.  This was somewhat of an abomination.
- It would get invalidated only if you deleted the handled-node (one past the pointed-to-node).
- It would get incremented if you deleted the pointed-to-node.
- rend() would get invalidated if you deleted the first node (final reverse node).

After my commits, ilist::reverse_iterator has similar behaviour to ilist::iterator (and std::{list,set,map}::{reverse_,}iterater).  The same goes for MachineBasicBlock::reverse_iterator, which is actually a higher-level wrapper around an ilist::reverse_iterator.


In order to try to find some of the code that relied on the old/broken behaviour, I didn't implement explicit constructors between ilist::iterator and ilist::reverse_iterator.
- We should probably do that at some point.
- I left a note saying so (which is perhaps what was found here).


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...

> On 2016-Dec-20, at 13:43, David Blaikie <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> 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
> 
> 
>