Skip to content

Simplify and refactor contractUncondEdges as a utility in TFCanonicalizeCFG. #19546

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Sep 26, 2018

Conversation

bgogul
Copy link
Contributor

@bgogul bgogul commented Sep 25, 2018

This PR also refactors the contractUncondBranches to reuse the mergeBasicBlockWithSuccessor(...) function from Utils/CFG.cpp

@bgogul
Copy link
Contributor Author

bgogul commented Sep 25, 2018

@swift-ci please test tensorflow

@bgogul bgogul requested a review from mhong September 25, 2018 23:31
// Our partitioning and other transformations can leave around lots of
// unconditional branches between blocks that formerly had control edges. Go
// through and merge those to make later passes simpler.
bool tf::contractUncondBranches(SILFunction *fn, DominanceInfo* DI, SILLoopInfo *LI) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will there be a caller where DI and LI are not NULL?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I am preparing a PR that passes non-null for DI & LI.

// while we walk it.
for (auto bbi = fn->begin(), e = fn->end(); bbi != e;) {
auto *bb = &*bbi;
if (mergeBasicBlockWithSuccessor(bb, DI, LI)) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the old code does not use mergeBasicBlockWithSuccessor() -- it may be worth mentioning this "less obvious refactoring" in PR description.

From a quick read of that func impl, it seems to be equivalent to the old code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the description.

@bgogul
Copy link
Contributor Author

bgogul commented Sep 26, 2018

Thanks for the review. Merging it now.

@bgogul bgogul merged commit 835215f into swiftlang:tensorflow Sep 26, 2018
@bgogul bgogul deleted the move_contract_edges branch September 26, 2018 21:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants