Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Enable Conv fusion optimizations in optimizeForIdeep #9255

Closed
wants to merge 4 commits into from
Closed

Enable Conv fusion optimizations in optimizeForIdeep #9255

wants to merge 4 commits into from

Conversation

gujinghui
Copy link
Collaborator

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

@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
@@ -101,6 +101,14 @@ std::vector<int> getKernelShape(std::map<std::string, caffe2::Argument> argMap)
return kernelShape;
}

int getGroup(std::map<std::string, caffe2::Argument> argMap) {

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
}

void OptimizeForIdeep(repr::NNModule* nn, caffe2::Workspace* ws, bool training_mode) {
if (training_mode) {

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

@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
return nullptr;
}

return dyn_cast<Caffe2Annotation>(annotation)->getOperatorDef();

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
return false;
auto convOutput = repr::nn::getOutputs(convNode).front();
auto consumers = repr::nn::getConsumers(convOutput);
// convOutput is NOT referenced by sequencial ops after BN.

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
break;
}
}
// Sum inputs should not be referenced by sequencial ops.

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
@@ -101,6 +101,14 @@ std::vector<int> getKernelShape(std::map<std::string, caffe2::Argument> argMap)
return kernelShape;
}

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

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
@@ -101,6 +101,14 @@ std::vector<int> getKernelShape(std::map<std::string, caffe2::Argument> argMap)
return kernelShape;
}

int getGroup(std::map<std::string, caffe2::Argument> argMap) {

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

@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

@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
@@ -42,6 +42,14 @@ std::vector<int> getDilations(std::map<std::string, caffe2::Argument> argMap) {
return dilations;
}

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

@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

@yinghai yinghai yinghai approved these changes

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