Skip to content

Enable Conv fusion optimizations in optimizeForIdeep#9255

Closed
gujinghui wants to merge 4 commits intopytorch:masterfrom
gujinghui:ideep_fusion
Closed

Enable Conv fusion optimizations in optimizeForIdeep#9255
gujinghui wants to merge 4 commits intopytorch:masterfrom
gujinghui:ideep_fusion

Conversation

@gujinghui
Copy link
Collaborator

@gujinghui gujinghui commented Jul 9, 2018

Enable fusion for IDEEP in optimizeForIdeep
including Conv+ReLU, Conv+Sum, Conv+Sum+ReLU, Conv+BN

@gujinghui gujinghui mentioned this pull request Jul 9, 2018
Closed
@gujinghui
Copy link
Collaborator Author

gujinghui commented Jul 9, 2018

@yinghai @jgong5

yinghai
yinghai reviewed Jul 9, 2018
View reviewed changes
Copy link
Contributor

@yinghai yinghai left a comment

Choose a reason for hiding this comment

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

Thanks for splitting it. Looks good overall. I have 2 minor comments regarding the interface.

caffe2/opt/converter.cc Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

caffe2/opt/optimize_ideep.cc Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

@gujinghui
Copy link
Collaborator Author

gujinghui commented Jul 11, 2018

@yinghai any other concerns?

facebook-github-bot
facebook-github-bot reviewed Jul 11, 2018
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@yinghai has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

yinghai
yinghai suggested changes Jul 11, 2018
View reviewed changes
Copy link
Contributor

@yinghai yinghai left a comment

Choose a reason for hiding this comment

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

Please clang-format your code.

caffe2/opt/optimize_ideep.cc Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

caffe2/opt/optimize_ideep.cc Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

caffe2/opt/optimize_ideep.cc Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

caffe2/opt/converter.cc Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

caffe2/opt/converter.cc Outdated

This comment was marked as off-topic.

Sign in to view

gujinghui added 2 commits July 12, 2018 10:13
@gujinghui
Enable Conv fusion optimizations in optimizeForIdeep
9eecae2 
Signed-off-by: Gu, Jinghui <jinghui.gu@intel.com>
@gujinghui
fix test case of conv_fusion op
1d801ed 
Signed-off-by: Gu, Jinghui <jinghui.gu@intel.com>
@gujinghui
Copy link
Collaborator Author

gujinghui commented Jul 12, 2018

@yinghai
code in convert.cc all use 'pass by value'.
Or, we have totally different code base?
Or, we'd better fix all of them...

@gujinghui
Refine code by clang-format
90213c0 
Signed-off-by: Gu, Jinghui <jinghui.gu@intel.com>
yinghai
yinghai approved these changes Jul 12, 2018
View reviewed changes
Copy link
Contributor

@yinghai yinghai left a comment

Choose a reason for hiding this comment

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

Let's have a separate pass to fix the pass-by-value issue.

facebook-github-bot
facebook-github-bot reviewed Jul 12, 2018
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@yinghai has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@gujinghui
Copy link
Collaborator Author

gujinghui commented Jul 13, 2018

@yinghai
This PR is under evaluation? When will it be merged?
Another patch depends on this one. Pls inform me if this is merged.
Thanks.

yinghai
yinghai suggested changes Jul 14, 2018
View reviewed changes
Copy link
Contributor

@yinghai yinghai left a comment

Choose a reason for hiding this comment

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

Sorry, Let's use CAFFE_ENFORCE

caffe2/opt/converter.cc

int getGroup(std::map<std::string, caffe2::Argument>& argMap) {
if (argMap.count("group")) {
CAFFE_ENFORCE(argMap["group"].has_i() && "Invalid group argument");

This comment was marked as off-topic.

Sign in to view

caffe2/opt/optimize_ideep.cc Outdated

Blob *getBlob(repr::NNGraph::NodeRef node, caffe2::Workspace *ws) {
auto tensor = repr::nn::get<repr::Tensor>(node);
assert(ws->HasBlob(tensor->getName()) && "Blob not in workspace");

This comment was marked as off-topic.

Sign in to view

@gujinghui
Use CAFFE_ENFORCE, instead of assert
aedefb8 
Signed-off-by: Gu, Jinghui <jinghui.gu@intel.com>
@gujinghui
Copy link
Collaborator Author

gujinghui commented Jul 15, 2018

@yinghai
fixed

facebook-github-bot
facebook-github-bot reviewed Jul 16, 2018
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@yinghai has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

goldsborough pushed a commit to goldsborough/pytorch that referenced this pull request Jul 20, 2018
@gujinghui @goldsborough
Enable Conv fusion optimizations in optimizeForIdeep (pytorch#9255)
931d1bc 
Summary:
Enable fusion for IDEEP in optimizeForIdeep
including Conv+ReLU, Conv+Sum, Conv+Sum+ReLU, Conv+BN
Pull Request resolved: pytorch#9255

Reviewed By: bddppq

Differential Revision: D8809030

Pulled By: yinghai

fbshipit-source-id: af30bad3b96cb965bd26a4dfa810370faec4bb88
jramseyer pushed a commit to jramseyer/pytorch that referenced this pull request Jul 30, 2018
@gujinghui @jramseyer
Enable Conv fusion optimizations in optimizeForIdeep (pytorch#9255)
c084897 
Summary:
Enable fusion for IDEEP in optimizeForIdeep
including Conv+ReLU, Conv+Sum, Conv+Sum+ReLU, Conv+BN
Pull Request resolved: pytorch#9255

Reviewed By: bddppq

Differential Revision: D8809030

Pulled By: yinghai

fbshipit-source-id: af30bad3b96cb965bd26a4dfa810370faec4bb88
goodlux pushed a commit to goodlux/pytorch that referenced this pull request Aug 15, 2018
@gujinghui
Enable Conv fusion optimizations in optimizeForIdeep (pytorch#9255)
e1d10d5 
Summary:
Enable fusion for IDEEP in optimizeForIdeep
including Conv+ReLU, Conv+Sum, Conv+Sum+ReLU, Conv+BN
Pull Request resolved: pytorch#9255

Reviewed By: bddppq

Differential Revision: D8809030

Pulled By: yinghai

fbshipit-source-id: af30bad3b96cb965bd26a4dfa810370faec4bb88
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@facebook-github-bot facebook-github-bot facebook-github-bot left review comments

+1 more reviewer

@yinghai yinghai yinghai approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

caffe2 open source

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@gujinghui @yinghai @facebook-github-bot @ezyang @weiyangfb