-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
Fix several bugs in compile time backward and Protobuf desc #4894
Conversation
and Rename `Sync` to `Flush`
Since lots of types can be cast to bool
…oung/Paddle into dev_impl_conv2d_layer
… dev_impl_conv2d_layer
… dev_impl_conv2d_layer
… fix_backward_bug
paddle/framework/block_desc.cc
Outdated
@@ -55,6 +55,12 @@ OpDescBind *BlockDescBind::AppendOp() { | |||
return ops_.back().get(); | |||
} | |||
|
|||
void BlockDescBind::AppendAllocatedOp(std::unique_ptr<OpDescBind> &op_desc) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is pretty hard to figure out the op_desc
has been changed inside AppendAllocatedOp
for others.
It is better to use std::move
outside, and accept the rvalue reference
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea!
paddle/framework/block_desc.cc
Outdated
void BlockDescBind::AppendAllocatedOp(std::unique_ptr<OpDescBind> &op_desc) { | ||
need_update_ = true; | ||
ops_.emplace_back(std::move(op_desc)); | ||
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return
is not needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
paddle/framework/block_desc.h
Outdated
OpDescBind *PrependOp(); | ||
|
||
std::vector<OpDescBind *> AllOps() const; | ||
|
||
int OpSize() const { return ops_.size(); } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return size_t
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
paddle/framework/block_desc.h
Outdated
OpDescBind *PrependOp(); | ||
|
||
std::vector<OpDescBind *> AllOps() const; | ||
|
||
int OpSize() const { return ops_.size(); } | ||
|
||
OpDescBind *Op(int idx) const { return ops_.at(idx).get(); } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use size_t as idx, also maybe do not mark this method as const
since it will change the program.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
paddle/pybind/protobuf.cc
Outdated
@@ -176,8 +176,10 @@ void BindBlockDesc(py::module &m) { | |||
py::return_value_policy::reference) | |||
.def("all_vars", &BlockDescBind::AllVars, | |||
py::return_value_policy::reference) | |||
.def("all_ops", &BlockDescBind::AllOps, | |||
py::return_value_policy::reference) | |||
// .def("all_ops", &BlockDescBind::AllOps, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basic LGTM, instead of some tiny enhancements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
splendid works. Thanks
fixes #4893
This PR is branched from #4868, so some changed may belong to that one and have no relationship with this topic.