[PATCH] D27782: [codegen] Add generic functions to skip debug values.

[Florian Hahn via Phabricator via llvm-commits]

fhahn marked 3 inline comments as done.
fhahn added a comment.

@iteratee I think the review you posted indeed a nice simplification in IfConversion.cpp. I think it would make sense to get your change in after this one goes in. What do you think?



================
Comment at: include/llvm/CodeGen/MachineBasicBlock.h:813
+template<typename IterT>
+inline bool skipDebugInstructionsForward(IterT &It, const IterT &End) {
+  while (It != End && It->isDebugValue())
----------------
MatzeB wrote:
> - Iterators are usually small enough to be better passed by value (so `IterT End` instead of `const IterT &End`).
> - APIs modifying their "parameters" through references are often unintuitive in C++. How about changing this to: `IterT skipDebugInstructionsForward(IterT Begin, IterT End);` that returns the new iterator position after skipping?
> - Given the iterator type is a template that I guess is only intended to be used with MachineBasicBlock::{const_}?{instr_}?iterator maybe listing those 4 possibiltites would help understanding the function.
Done. Is the reference to the iterators in the comment along the lines you meant?


================
Comment at: include/llvm/CodeGen/MachineBasicBlock.h:829
+    It--;
+  return It == Begin && It->isDebugValue();
+}
----------------
iteratee wrote:
> This is subtly different from the code you're replacing. Specifically if, It == Begin, and Begin is not a debug value, this function returns false, where shrinkInclusiveRange returns true. You need to early return if It == Begin because the resulting range is now empty.
I've changed the function to return the new iterator and the calling functions must interpret the returned iterator.


https://reviews.llvm.org/D27782