This is an archive of the discontinued LLVM Phabricator instance.

IfConversion: Use reverse_iterator to simplify. NFC
ClosedPublic

Authored by iteratee on Dec 14 2016, 6:07 PM.

Details

Reviewers
dblaikie
fhahn
Summary

This simplifies skipping debug instructions and shrinking ranges.

Diff Detail

Event Timeline

iteratee updated this revision to Diff 81515.Dec 14 2016, 6:07 PM
iteratee retitled this revision from to IfConversion: Use reverse_iterator to simplify. NFC.
iteratee updated this object.
iteratee added a reviewer: fhahn.
iteratee set the repository for this revision to rL LLVM.
iteratee added subscribers: llvm-commits, timshen.
fhahn edited edge metadata.Dec 18 2016, 1:21 PM

Now that D27782 is merged, I think this would be a good simplification to get in, but the patch needs rebasing.

iteratee updated this revision to Diff 82042.Dec 19 2016, 5:04 PM
iteratee edited edge metadata.
iteratee removed rL LLVM as the repository for this revision.

Rebased as requested.

dblaikie added inline comments.
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?

fhahn added inline comments.Dec 20 2016, 6:07 AM
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.

iteratee updated this revision to Diff 82135.Dec 20 2016, 12:51 PM

Changes from comments.

iteratee updated this revision to Diff 82138.Dec 20 2016, 1:00 PM
iteratee marked 2 inline comments as done.

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.

iteratee updated this revision to Diff 83338.Jan 5 2017, 6:49 PM

Are there any other concerns with this patch?

lib/CodeGen/IfConversion.cpp
713–721

I'd rather not here.

fhahn added inline comments.Jan 6 2017, 1:02 AM
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.

iteratee updated this revision to Diff 83389.Jan 6 2017, 11:20 AM
iteratee marked an inline comment as done.

Combine some end checks.

Ping? Anything else on this?

I think all comments have been addressed properly. @dblaikieCould you have another look?

dblaikie accepted this revision.Jan 25 2017, 9:37 AM

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?

This revision is now accepted and ready to land.Jan 25 2017, 9:37 AM
iteratee added inline comments.Jan 26 2017, 11:55 AM
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.

fhahn added a comment.Jan 31 2017, 3:39 PM

It looks

lib/CodeGen/IfConversion.cpp
647

That makes sense. @dblaikie do you think we should check with Duncan before committing this?

iteratee closed this revision.Jan 31 2017, 3:44 PM

Committed in rL293202