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.

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

Add BlockDesc and ProgramDesc to framework.proto #4276

Merged
merged 5 commits into from
Sep 21, 2017

Conversation

JiayiFeng
Copy link
Collaborator

No description provided.

@JiayiFeng JiayiFeng requested a review from reyoung September 21, 2017 00:57
@@ -29,11 +29,13 @@ namespace framework {

typedef boost::variant<boost::blank, int, float, std::string, std::vector<int>,
std::vector<float>, std::vector<std::string>,
std::vector<std::pair<int, int>>>
std::vector<std::pair<int, int>>, BlockDesc>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually, this variant is BlockDesc* to avoid memcpy.

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

Attribute;

typedef std::unordered_map<std::string, Attribute> AttributeMap;

static ProgramDesc g_program_desc;
Copy link
Collaborator

Choose a reason for hiding this comment

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

In Google C++ style, a class instance cannot be a global variable.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It cannot be written in .h

Copy link
Collaborator

Choose a reason for hiding this comment

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

Variable definition usually should not be written in a header file, because it will define all variables in .cc which includes this header. Here should be a declaration.

  • To define an int value, we use int a;. To declare an int value, we use extern int a;. extern means somewhere is define that int value, but every .cc which includes this header can use that int value.

  • static means not to generate symbol name in .S or .o file. So this will actually create many ProgramDesc, each .cc has a new g_program_desc.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Got it

std::vector<std::pair<int, int>> val(attr_desc.int_pairs_size());
for (int i = 0; i < attr_desc.int_pairs_size(); ++i) {
val[i].first = attr_desc.int_pairs(i).first();
val[i].second = attr_desc.int_pairs(i).second();
}
return val;
}
case framework::AttrType::BLOCK: {
return g_program_desc.blocks(attr_desc.block_idx());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just return a pointer, to avoid memcpy.

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

@reyoung reyoung left a comment

Choose a reason for hiding this comment

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

Excellent Job!

@JiayiFeng JiayiFeng merged commit 7d33447 into PaddlePaddle:develop Sep 21, 2017
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