-
Notifications
You must be signed in to change notification settings - Fork 5.9k
[PIR] support attach interface for OpInfo. #59369
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
Conversation
|
你的PR提交成功,感谢你对开源项目的贡献! |
|
❌ The PR is not created using PR's template. You can refer to this Demo. |
5a33c1c to
5ef4bfa
Compare
5ef4bfa to
75a38a0
Compare
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.
LGTM overrall
| const std::vector<std::vector<pir::OpResult>> &outputs, | ||
| const std::vector<std::vector<pir::Value>> &out_grads, | ||
| const std::vector<std::vector<bool>> &stop_gradients) { | ||
| PADDLE_ENFORCE_EQ(inputs.size() == 1u && inputs[0].size() >= 1u, |
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.
这个判断有必要么?
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.
我觉得判断一下比较好,不然后面inputs[0][0]可能会直接崩溃,不好发现原因
| (void)std::initializer_list<int>{ | ||
| 0, (PlacementConstrctInterface<Args>(p_interface), 0)...}; | ||
| return p_interface; | ||
| 0, (PlacementConstrctInterface<Args>(interface_set), 0)...}; |
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.
| 0, (PlacementConstrctInterface<Args>(interface_set), 0)...}; | |
| 0, (ConstrctInterface<Args>(interface_set), 0)...}; |
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.
好的,这个名字我在下个pr中修改。
| const char **p_attributes, | ||
| VerifyPtr verify_sig, | ||
| VerifyPtr verify_region) | ||
| : interface_set_(std::move(interface_set)), |
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.
将interface_set_调整到num_traits后面或前面会更好?
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.
个人觉得占用空间最大的成员变量放在最前面会比较好。我们可以再讨论一下。
PR types
New features
PR changes
Others
Description
Other
Pcard-67164