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

[C++ API GetAllocator] Add C++ GetAllocator interface #50813

Merged
merged 2 commits into from
Feb 28, 2023
Merged

[C++ API GetAllocator] Add C++ GetAllocator interface #50813

merged 2 commits into from
Feb 28, 2023

Conversation

jiahy0825
Copy link
Contributor

PR types

New features

PR changes

APIs

Describe

Add C++ GetAllocator interface, which enables acquiring Allocator* directly.

@paddle-bot
Copy link

paddle-bot bot commented Feb 23, 2023

你的PR提交成功，感谢你对开源项目的贡献!
请关注后续CI自动化测试结果，详情请参考Paddle-CI手册。
Your PR has been submitted. Thanks for your contribution!
Please wait for the result of CI firstly. See Paddle CI Manual for details.

zyfncg
zyfncg reviewed Feb 27, 2023
View reviewed changes
paddle/phi/core/CMakeLists.txt Outdated
@@ -49,7 +49,7 @@ cc_library(
cc_library(
tensor_base
SRCS tensor_base.cc allocator.cc
DEPS phi_enforce)
DEPS phi_enforce context_pool phi_device_context)
Copy link
Contributor

Choose a reason for hiding this comment

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

这里看上去会有循环依赖，GetAllocator可能需要换到其他文件里

Copy link
Contributor Author

Choose a reason for hiding this comment

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

线下讨论后，paddle/phi/core/allocator.h 用于底层的 Allocation 定义，上层的 GetAllocator API 接口放到 api 目录下的 context_pool.h 文件中

After offline discussion, paddle/phi/core/allocator.h is specific for the definition of Allocation, on the other hand, the upper-level GetAllocator API will be moved to phi/context_pool.h.

YuanRisheng
YuanRisheng reviewed Feb 27, 2023
View reviewed changes
paddle/phi/tests/common/test_allocator.cu Outdated
See the License for the specific language governing permissions and
limitations under the License. */

#include <gtest/gtest.h>
Copy link
Contributor

Choose a reason for hiding this comment

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

这个单测应该放在core目录下吧？

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks~GetAllocator 移动到了 api 目录下，单测也相应移动到 tests/api 目录

Thanks～As GetAllocator has been moved to api directory, the corresponding unit test moves to test/api directory.

@jiahy0825 jiahy0825 closed this Feb 28, 2023
@jiahy0825 jiahy0825 reopened this Feb 28, 2023
zyfncg
zyfncg approved these changes Feb 28, 2023
View reviewed changes
paddle/phi/core/allocator.cc
Comment on lines +17 to +19
#include "paddle/phi/api/include/context_pool.h"
#include "paddle/phi/core/device_context.h"

Copy link
Contributor

Choose a reason for hiding this comment

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

这两个头文可以移除

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Next PR fix this.

paddle/phi/core/allocator.h
@@ -18,6 +18,7 @@ limitations under the License. */
#include <functional>
#include <memory>

#include "paddle/phi/api/include/dll_decl.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

同上

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as above. Next PR fix this.

paddle/phi/api/include/context_pool.h
@@ -87,3 +87,10 @@ class PADDLE_API DeviceContextPool {

} // namespace experimental
} // namespace paddle

namespace phi {
Copy link
Contributor

Choose a reason for hiding this comment

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

这里需要决策下是以phi还是paddle的namespace暴露到外部

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Next PR fix this.
After offline discussion, we decide to use the namespace paddle

@jiahy0825 jiahy0825 merged commit 74446b3 into PaddlePaddle:develop Feb 28, 2023
jiahy0825 added a commit to jiahy0825/Paddle that referenced this pull request Feb 28, 2023
@jiahy0825
polish codes according PaddlePaddle#50813
43d4555
jiahy0825 added a commit that referenced this pull request Mar 2, 2023
@jiahy0825
[GetCurrentCUDAStream] Add C++ API GetCurrentCUDAStream (#51027)
cce2b94 
* polish codes according #50813

* [getCurrentCUDAStream] Add C++ API getCurrentCUDAStream

* change get->Get

* wrap with macro

* use Get instead of get
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@YuanRisheng YuanRisheng YuanRisheng approved these changes

@chenwhql chenwhql chenwhql approved these changes

@zyfncg zyfncg zyfncg approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jiahy0825 @zyfncg @chenwhql @YuanRisheng