Skip to content

Rewrite inplace pass and fix gc bug#17126

Merged
sneaxiy merged 6 commits intoPaddlePaddle:developfrom
sneaxiy:fix_op_graph_view
Apr 30, 2019
Merged

Rewrite inplace pass and fix gc bug#17126
sneaxiy merged 6 commits intoPaddlePaddle:developfrom
sneaxiy:fix_op_graph_view

Conversation

@sneaxiy
Copy link
Collaborator

@sneaxiy sneaxiy commented Apr 27, 2019

No description provided.

@sneaxiy sneaxiy changed the title Fix op graph view Rewrite inplace pass and fix gc bug Apr 29, 2019
@sneaxiy sneaxiy force-pushed the fix_op_graph_view branch 2 times, most recently from d6c5ac8 to 028e920 Compare April 29, 2019 10:37
test=develop
@sneaxiy sneaxiy force-pushed the fix_op_graph_view branch from 028e920 to 0a47464 Compare April 29, 2019 10:51
Copy link
Member

Choose a reason for hiding this comment

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

Tiny Grammar issue: "each other name" should be "each other's name"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

Copy link
Member

Choose a reason for hiding this comment

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

Grammar: "avoid doing"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

Copy link
Member

Choose a reason for hiding this comment

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

Oh >.< , I meant "avoid optimizing the variable ..."

Copy link
Member

Choose a reason for hiding this comment

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

I saw you did Op()->Flush() after renaming at this function above. Do we need to do Flush() after renaming here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

std::unique_ptr<ir::Graph> test_SingleOpInplaceInToOut(
std::unique_ptr<ir::Graph> g) {
std::unique_ptr<details::InplacePass> pass(new details::InplacePass());
auto pass = ir::PassRegistry::Instance().Get("inplace_pass");
Copy link
Member

Choose a reason for hiding this comment

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

"auto pass = CreateInplacePass()" to de-duplicate code?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

std::queue<OpHandleBase *> q;
std::unordered_set<OpHandleBase *> visited;
q.push(op);
do {
Copy link
Member

Choose a reason for hiding this comment

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

Prefer "while" loop than "do while" loop.

There is q.push(op) before the while loop. It's no harm to write
while (!q.empty()) {
...
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

@@ -65,6 +65,7 @@ bool OpGraphView::VisitAllPendingOps(OpHandleBase *op,
if (!callback(pending_op)) {
return false;
}
Copy link
Member

Choose a reason for hiding this comment

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

Prefer "while" loop than "do while" loop.

There is q.push(op) before the while loop. It's no harm to write
while (!q.empty()) {
...
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

zhhsplendid
zhhsplendid previously approved these changes Apr 30, 2019
Copy link
Member

@zhhsplendid zhhsplendid left a comment

Choose a reason for hiding this comment

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

LGTM but please change the "avoid doing optimize" to "avoid optimizing" which I commented.

@sneaxiy
Copy link
Collaborator Author

sneaxiy commented Apr 30, 2019

@zhhsplendid Thanks. I have change "avoid dong optimize" to be "avoid optimizing".

Copy link
Contributor

@chengduoZH chengduoZH left a comment

Choose a reason for hiding this comment

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

LGTM

@sneaxiy sneaxiy merged commit 4e1bc6e into PaddlePaddle:develop Apr 30, 2019
@sneaxiy sneaxiy deleted the fix_op_graph_view branch October 17, 2019 07:09
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.

3 participants