Page MenuHomePhabricator

[codegen] Add generic functions to skip debug values.

Authored by fhahn on Dec 14 2016, 3:36 PM.



This commits moves skipDebugInstructionsForward and
skipDebugInstructionsBackward from lib/CodeGen/IfConversion.cpp
to include/llvm/CodeGen/MachineBasicBlock.h and updates
some codgen files to use them.

This refactoring was suggested in
and I thought it's best to do the refactoring in a separate
review, but I could also put both changes in a single review
if that's preferred.

Also, the names for the functions aren't the snappiest and
I would be happy to rename them if anybody has suggestions.

Diff Detail

Event Timeline

fhahn updated this revision to Diff 81490.Dec 14 2016, 3:36 PM
fhahn retitled this revision from to [codegen] Add generic functions to skip debug values..
fhahn updated this object.
fhahn added reviewers: iteratee, aprantl, eli.friedman.
fhahn added a subscriber: llvm-commits.
aprantl accepted this revision.Dec 14 2016, 3:44 PM
aprantl edited edge metadata.

Small formatting nit, but otherwise this looks much better!


Nitpick: we use \param instead of @param

This revision is now accepted and ready to land.Dec 14 2016, 3:44 PM
aprantl added inline comments.Dec 14 2016, 3:45 PM

--CurrPos; ?

Thanks for doing the refactoring. I have some comments on the API:

  • 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.
iteratee added inline comments.Dec 14 2016, 4:45 PM

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.

MatzeB added inline comments.Dec 14 2016, 4:46 PM

Not wanting to be too nitpicky or criticize for actually writing good documentation, but I can't stop thinking that some cases where a comment reads like \param It The iterator to use is not that far off from a comment like int i; // an integer variable, which doesn't really add much value. So I often simply leave out the \param stuff when things are sufficiently clearly explained in the description part.

iteratee edited edge metadata.Dec 14 2016, 6:03 PM

I have a counter CL to simplify the skipping in a different way. Can you look at that before you submit this?

Please take a look at I think we can make this even more simple.

fhahn updated this revision to Diff 81684.Dec 15 2016, 4:11 PM
fhahn edited edge metadata.

Thank you for the helpful comments. I hope I addressed them all.

fhahn marked 3 inline comments as done.Dec 15 2016, 4:17 PM

@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?


Done. Is the reference to the iterators in the comment along the lines you meant?


I've changed the function to return the new iterator and the calling functions must interpret the returned iterator.

MatzeB accepted this revision.Dec 15 2016, 4:20 PM
MatzeB added a reviewer: MatzeB.



extra space before the comma.

fhahn updated this revision to Diff 81726.Dec 16 2016, 2:32 AM
fhahn edited edge metadata.

style nitpick

fhahn closed this revision.Dec 16 2016, 3:20 AM