This is an archive of the discontinued LLVM Phabricator instance.

Codegen: Don't tail-duplicate blocks with un-analyzable fallthrough.
ClosedPublic

Authored by iteratee on May 24 2016, 3:39 PM.

Details

Reviewers
echristo
haicheng
Summary

If AnalyzeBranch can't analyze a block and it is possible to
fallthrough, then duplicating the block doesn't make sense, as only one
block can be the layout predecessor for the un-analyzable fallthrough.

Submitted wit a test case, but NOTE: the test case doesn't currently
fail. However, the test case fails with D20505 and would have saved me
some time debugging.

Diff Detail

Event Timeline

iteratee updated this revision to Diff 58341.May 24 2016, 3:39 PM
iteratee retitled this revision from to Codegen: Don't tail-duplicate blocks with un-analyzable fallthrough..
iteratee updated this object.
iteratee added a reviewer: haicheng.
iteratee set the repository for this revision to rL LLVM.
iteratee added subscribers: llvm-commits, echristo.
echristo accepted this revision.May 24 2016, 3:42 PM
echristo added a reviewer: echristo.

Couple of random drive by testcase changes, and a comment request, otherwise looks OK to me.

-eric

lib/CodeGen/TailDuplicator.cpp
595

Might want to comment that this matches the other test as well.

test/CodeGen/PowerPC/tail-dup-analyzable-fallthrough.ll
60

Can probably limit the flags here.

62–69

Probably don't need the TBAA information?

This revision is now accepted and ready to land.May 24 2016, 3:42 PM
iteratee updated this revision to Diff 58356.May 24 2016, 4:51 PM
iteratee edited edge metadata.
iteratee removed rL LLVM as the repository for this revision.

Tidied up test case.

iteratee added inline comments.May 24 2016, 4:51 PM
lib/CodeGen/TailDuplicator.cpp
595

Do you mean that it matches the test in MachineBlockPlacement.cpp?

echristo added inline comments.May 24 2016, 5:08 PM
lib/CodeGen/TailDuplicator.cpp
595

Yep!

iteratee abandoned this revision.May 25 2016, 1:35 PM

I just realized that this isn't a bug in the current code, as fallthrough doesn't get duplicated currently, but only a bug with D20505. I'm going to roll this fix in there along with the test case.

iteratee updated this revision to Diff 65849.Jul 27 2016, 6:19 PM

Fixed test case. Existing test case relied on tail-merging. This one doesn't.

This revision is now accepted and ready to land.Jul 27 2016, 6:19 PM
iteratee updated this revision to Diff 65852.Jul 27 2016, 6:22 PM

Expanded comment.

Am I still OK to commit this?

iteratee closed this revision.Aug 11 2016, 2:21 PM

If it has been committed, can you post the revision number here when closing please?

Committed in r278287

Scratch that. Committed in r278288