This simplifies skipping debug instructions and shrinking ranges.
Diff Detail
Event Timeline
Now that D27782 is merged, I think this would be a good simplification to get in, but the patch needs rebasing.
lib/CodeGen/IfConversion.cpp | ||
---|---|---|
644–651 | 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? | |
646–649 | 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? | |
713–721 | auto? |
lib/CodeGen/IfConversion.cpp | ||
---|---|---|
679 | If you are going to use std::next at line 648-651, it would be good to use it here as well to keep it consistent. I would prefer to use std::next personally. |
I swear I need to teach git format-patch to warn if creating a patch from HEAD and a file in the patch is modified.
lib/CodeGen/IfConversion.cpp | ||
---|---|---|
644–651 | See the comment in include/llvm/ADT/ilist_iterator.h: /// Convert from an iterator to its reverse. /// /// TODO: Roll this into the implicit constructor once we're sure that no one /// is relying on the std::reverse_iterator off-by-one semantics. I take that to mean that we are intentionally not supplying the std::reverse_iterator semantics. The comment is aimed at a reader familiar with those semantics. |
Are there any other concerns with this patch?
lib/CodeGen/IfConversion.cpp | ||
---|---|---|
713–721 | I'd rather not here. |
lib/CodeGen/IfConversion.cpp | ||
---|---|---|
623 | I think we could first execute both TIB = skipDebugInstructionsForward(TIB, TIE); and FIB = skipDebugInstructionsForward(FIB, FIE); and then break from a single if. You already do that at line 722. |
I think all comments have been addressed properly. @dblaikieCould you have another look?
Sure - not sure what Duncan's preference is here, or the ultimate end goal re: consistency, etc, is, though.
lib/CodeGen/IfConversion.cpp | ||
---|---|---|
647 | This construct is used rather than this: MachineBasicBlock::reverse_iterator RTIE = TIE; to avoid depending on that conversion & it's subtle off-by-one jump? |
lib/CodeGen/IfConversion.cpp | ||
---|---|---|
647 | Currently, there is no such conversion in ilist_iterator, only a getReverse() method, that doesn't perform an off-by-one jump. |
I think we could first execute both TIB = skipDebugInstructionsForward(TIB, TIE); and FIB = skipDebugInstructionsForward(FIB, FIE); and then break from a single if. You already do that at line 722.