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

Florian Hahn via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Dec 15 16:17:33 PST 2016

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.


More information about the llvm-commits mailing list