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

[Kyle Butt via Phabricator via llvm-commits]

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