[STLExtras] Add a two argument form of make_early_inc_range

[foad]

Summary

It seems convenient to me to be able to use make_early_inc_range as a
direct replacement for make_range, so you can change:

make_range(X, Y)

into:

make_early_inc_range(X, Y)

instead of:

make_early_inc_range(make_range(X, Y))

[foad]

This is an RFC. I'm not really sure if it's worth doing, it just seemed natural to me that make_range and make_early_inc_range could be used in the same way.

[silvas]

Seems fine to me in principle. Wait for others to weigh in though. @rriddle @dexonsmith

[dexonsmith]

In D101621#2730860, @silvas wrote:

Seems fine to me in principle. Wait for others to weigh in though. @rriddle @dexonsmith

I agree, this seems nice / uncontroversial. @dblaikie reviews much of the range stuff, might be worth giving him some time to come across it too.

[dexonsmith]

Might be worth adding a small unit test, besides the examples in code, although I'm not sure what we usually do for simple wrappers like this...

[dblaikie]

In D101621#2728784, @foad wrote:

This is an RFC. I'm not really sure if it's worth doing, it just seemed natural to me that make_range and make_early_inc_range could be used in the same way.

Agreed they could be used in this way (existence proofs are the callsites you've updated in this patch) - but I'm not sure they need a wrapper that does both together. I rather like the orthogonality/composability of things. But I wouldn't insist this never be done - just vague preference to leaving them as separate things that callers can compose.

In D101621#2730889, @dexonsmith wrote:

Might be worth adding a small unit test, besides the examples in code, although I'm not sure what we usually do for simple wrappers like this...

Yeah, I think we're pretty hit-or-miss on testing such small things. I'm about where you are on this (if this patch is committed) - might be nice to have a unit test, but not the end of the world in any case.

[dblaikie]

A few extra words: Generally I'd hope we move towards fewer uses of make_range (& mostly in the implementation of container-like things, rather than in random code handling iterators, like in the two places touched in this patch) and more towards having ranges around for a while to begin with.

For instance, the second piece of code, in LoopInterchange, might be nice if we had a "drop last" operation that operated on a range and this code became:

make_early_inc_range(drop_last(*InnerLoopPreHeader))

[foad]

In D101621#2730920, @dblaikie wrote:

In D101621#2728784, @foad wrote:

This is an RFC. I'm not really sure if it's worth doing, it just seemed natural to me that make_range and make_early_inc_range could be used in the same way.

Agreed they could be used in this way (existence proofs are the callsites you've updated in this patch) - but I'm not sure they need a wrapper that does both together. I rather like the orthogonality/composability of things. But I wouldn't insist this never be done - just vague preference to leaving them as separate things that callers can compose.

I'd be fine with that except that the name make_<something>_range strongly suggests to me that it should behave in the same kind of way as make_range, just making some specific kind of range instead.

[dblaikie]

In D101621#2731274, @foad wrote:

In D101621#2730920, @dblaikie wrote:

In D101621#2728784, @foad wrote:

This is an RFC. I'm not really sure if it's worth doing, it just seemed natural to me that make_range and make_early_inc_range could be used in the same way.

Agreed they could be used in this way (existence proofs are the callsites you've updated in this patch) - but I'm not sure they need a wrapper that does both together. I rather like the orthogonality/composability of things. But I wouldn't insist this never be done - just vague preference to leaving them as separate things that callers can compose.

I'd be fine with that except that the name make_<something>_range strongly suggests to me that it should behave in the same kind of way as make_range, just making some specific kind of range instead.

Oh, sure - though I think the lesson to take from that is that the abstraction is correct (because composability/orthogonality is good) and the name is less than ideal. Totally open to naming suggestions/renaming this thing.

[fhahn]

Marking as changes requested as there are outstanding comments.

[lebedev.ri]

This review may be stuck/dead, consider abandoning if no longer relevant.
Removing myself as reviewer in attempt to clean dashboard.

[foad]

Maybe make_early_inc_range could be renamed to something like early_increment but I'm not sufficiently motivated to try that now.

llvm/include/llvm/ADT/STLExtras.h

 iterator_range<early_inc_iterator_impl<detail::IterOfRange<RangeT>>>
 make_early_inc_range(RangeT &&Range) {
   using EarlyIncIteratorT =
       early_inc_iterator_impl<detail::IterOfRange<RangeT>>;
   return make_range(EarlyIncIteratorT(std::begin(std::forward<RangeT>(Range))),
                     EarlyIncIteratorT(std::end(std::forward<RangeT>(Range))));
 }
 
+/// Make a range that does early increment given the start and end iterators.
+/// This is a convenience form of make_early_inc_range(Range) which allows
+/// make_early_inc_range to be used as a direct replacement for make_range.
+template <typename IteratorT>
+iterator_range<
+    early_inc_iterator_impl<detail::IterOfRange<iterator_range<IteratorT>>>>
+make_early_inc_range(IteratorT X, IteratorT Y) {
+  return make_early_inc_range(make_range(X, Y));
+}
+
 // forward declarations required by zip_shortest/zip_first/zip_longest
 template <typename R, typename UnaryPredicate>
 bool all_of(R &&range, UnaryPredicate P);
 template <typename R, typename UnaryPredicate>
 bool any_of(R &&range, UnaryPredicate P);
 
 namespace detail {
 

llvm/lib/CodeGen/SelectionDAG/ScheduleDAGSDNodes.cpp

EmitSchedule(MachineBasicBlock::iterator &InsertPos) {
   // In some cases, DBG_VALUEs might be inserted after the first terminator,
   // which results in an invalid MBB. If that happens, move the DBG_VALUEs
   // before the first terminator.
   MachineBasicBlock *InsertBB = Emitter.getBlock();
   auto FirstTerm = InsertBB->getFirstTerminator();
   if (FirstTerm != InsertBB->end()) {
     assert(!FirstTerm->isDebugValue() &&
            "first terminator cannot be a debug value");
-    for (MachineInstr &MI : make_early_inc_range(
-             make_range(std::next(FirstTerm), InsertBB->end()))) {
+    for (MachineInstr &MI :
+         make_early_inc_range(std::next(FirstTerm), InsertBB->end())) {
       if (!MI.isDebugValue())
         continue;
 
       if (&MI == InsertPos)
         InsertPos = std::prev(InsertPos->getIterator());
 
       // The DBG_VALUE was referencing a value produced by a terminator. By
       // moving the DBG_VALUE, the referenced value also needs invalidating.

llvm/lib/Transforms/Scalar/LoopInterchange.cpp

bool LoopInterchangeTransform::transform() {
   // defined in the outer loop header. Move them there, because the original
   // inner loop preheader will become the entry into the interchanged loop nest.
   // Currently we move all instructions and rely on LICM to move invariant
   // instructions outside the loop nest.
   BasicBlock *InnerLoopPreHeader = InnerLoop->getLoopPreheader();
   BasicBlock *OuterLoopHeader = OuterLoop->getHeader();
   if (InnerLoopPreHeader != OuterLoopHeader) {
     SmallPtrSet<Instruction *, 4> NeedsMoving;
-    for (Instruction &I :
-         make_early_inc_range(make_range(InnerLoopPreHeader->begin(),
-                                         std::prev(InnerLoopPreHeader->end()))))
+    for (Instruction &I : make_early_inc_range(
+             InnerLoopPreHeader->begin(), std::prev(InnerLoopPreHeader->end())))
       I.moveBefore(OuterLoopHeader->getTerminator());
   }
 
   Transformed |= adjustLoopLinks();
   if (!Transformed) {
     LLVM_DEBUG(dbgs() << "adjustLoopLinks failed\n");
     return false;
   }