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

[David Blaikie via Phabricator via llvm-commits]

dblaikie 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());
----------------
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?


================
Comment at: lib/CodeGen/IfConversion.cpp:648-651
+  MachineBasicBlock::reverse_iterator RTIE = ++(TIE.getReverse());
+  MachineBasicBlock::reverse_iterator RFIE = ++(FIE.getReverse());
+  const MachineBasicBlock::reverse_iterator RTIB = ++(TIB.getReverse());
+  const MachineBasicBlock::reverse_iterator RFIB = ++(FIB.getReverse());
----------------
Unnecessary ()

Also, if those functions return by value, I don't think you can reliably ++ an iterator value (works for user defined types/operator overloads, but not for primitive types like 'int*' for example).

Maybe std::next would be better on both counts?


================
Comment at: lib/CodeGen/IfConversion.cpp:716-719
+  const MachineBasicBlock::reverse_iterator B1 = MBB1->rend();
+  const MachineBasicBlock::reverse_iterator B2 = MBB2->rend();
+  MachineBasicBlock::reverse_iterator E1 = MBB1->rbegin();
+  MachineBasicBlock::reverse_iterator E2 = MBB2->rbegin();
----------------
auto?


https://reviews.llvm.org/D27792