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

[pten] add concat pten kernel #38955

Merged

Conversation

MingMingShangTian
Copy link
Contributor

PR types

Others

PR changes

Others

Describe

add concat pten kernel

@paddle-bot-old
Copy link

Thanks for your contribution!
Please wait for the result of CI firstly. See Paddle CI Manual for details.

@@ -13,6 +13,8 @@ See the License for the specific language governing permissions and
limitations under the License. */

#include "paddle/fluid/operators/math/concat_and_split.h"

#include "paddle/pten/kernels/cpu/concat_and_split.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

建议直接把concat_and_split.cc迁移过来,马上我们也要把它移过来了,可以下个PR再做

Copy link
Contributor Author

Choose a reason for hiding this comment

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

好的

#include "paddle/pten/kernels/funcs/concat_funcs.h"
namespace pten {

DenseTensorMeta ConcatInferMeta(const std::vector<DenseTensorMeta>& x_meta,
Copy link
Contributor

Choose a reason for hiding this comment

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

这里后面会改成的返回值作为输入参数指针的形式,和kernel保持一致

Copy link
Contributor Author

Choose a reason for hiding this comment

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

收到


DenseTensorMeta ConcatInferMeta(const std::vector<DenseTensorMeta>& x_meta,
const Scalar& axis_scalar,
bool is_runtime) {
Copy link
Contributor

Choose a reason for hiding this comment

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

is_runtime后面会用一个结构体封装起来

Copy link
Contributor Author

Choose a reason for hiding this comment

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

好的

#include "paddle/pten/core/tensor_meta.h"
namespace pten {

// TODO(chentianyu03) use std::vector<DenseTensor> as InferMeta inputs
Copy link
Contributor

Choose a reason for hiding this comment

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

这里后面会新增MetaTensor概念,作为inferMeta的输入

Copy link
Contributor Author

Choose a reason for hiding this comment

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

好的,后续再调整

#include "paddle/fluid/framework/mixed_vector.h"
#include "paddle/fluid/memory/malloc.h"
#include "paddle/fluid/operators/math/concat_and_split.h"
#include "paddle/fluid/platform/cuda_graph_with_memory_pool.h"
Copy link
Contributor

@chenwhql chenwhql Jan 18, 2022

Choose a reason for hiding this comment

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

[TODO] 这里需要梳理下platform下还依赖了哪些组件,是需要我们提前迁移的

Copy link
Contributor Author

Choose a reason for hiding this comment

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

好的

#include <vector>
#include "gflags/gflags.h"
#include "paddle/fluid/framework/mixed_vector.h"
#include "paddle/fluid/memory/malloc.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

这里依赖malloc有点严重,看下我们有替代写法吗

Copy link
Contributor Author

Choose a reason for hiding this comment

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

后续优化

tmp_dev_ins_data = paddle::memory::Alloc(context, in_num * sizeof(T*));
auto* restored = paddle::platform::RestoreHostMemIfCapturingCUDAGraph(
inputs_data, in_num);
paddle::memory::Copy(context.GetPlace(),
Copy link
Contributor

Choose a reason for hiding this comment

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

能否使用copy_kernel

Copy link
Contributor Author

Choose a reason for hiding this comment

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

copy_kernle 是对tensor的操作,这里使用的是指针地址


#include "paddle/pten/kernels/concat_kernel.h"

#include "paddle/fluid/framework/lod_tensor.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

为什么还需要lod_tensor,这里的头文件确认下必要性

Copy link
Contributor Author

@MingMingShangTian MingMingShangTian Jan 18, 2022

Choose a reason for hiding this comment

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

这里引用lod_tensor.h,是因为使用了AppendLoD 等辅助函数

@@ -0,0 +1,86 @@
/* Copyright (c) 2021 PaddlePaddle Authors. All Rights Reserved.
Copy link
Contributor

Choose a reason for hiding this comment

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

2022

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

std::vector<pten::DenseTensor> pt_tensors;

for(auto & t : tensors) {
pt_tensors.push_back(*std::dynamic_pointer_cast<pten::DenseTensor>(t.impl()));
Copy link
Contributor

Choose a reason for hiding this comment

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

这里避免使用dynamic_cast,使用is_dense_tensor判断+static cast

Copy link
Contributor Author

Choose a reason for hiding this comment

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

已沟通,先保持现状

Comment on lines 73 to 74
auto in_lod = paddle::framework::ConvertToLengthBasedLoD(x[i].lod());
paddle::framework::AppendLoD(out_lod, in_lod);
Copy link
Contributor

Choose a reason for hiding this comment

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

这个kernel文件里引入较多fluid下的代码,建议评估一下迁移难度,如果可以尽量将依赖函数迁移到pten下

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

chenwhql
chenwhql previously approved these changes Jan 20, 2022
YuanRisheng
YuanRisheng previously approved these changes Jan 20, 2022
Copy link
Contributor

@XiaoguangHu01 XiaoguangHu01 left a comment

Choose a reason for hiding this comment

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

LGTM

@Shixiaowei02 Shixiaowei02 merged commit 06803c2 into PaddlePaddle:develop Jan 21, 2022
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.

5 participants