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

Migrate unstack stack range unique op into Phi #40851

Merged
merged 1 commit into from
Mar 31, 2022

Conversation

csy0225
Copy link
Contributor

@csy0225 csy0225 commented Mar 23, 2022

PR types

Function optimization

PR changes

OPs

Describe

[Phi] Migrate unstack stack range unique op into Phi

@csy0225 csy0225 force-pushed the unstack_range_stack_unique branch 2 times, most recently from 2e8109f to d60bd3d Compare March 23, 2022 11:36
Comment on lines +47 to +49
#define _PhiForEachDataTypeTiny_(callback) \
_PhiForEachDataTypeHelper_(callback, int, DataType::INT32); \
_PhiForEachDataTypeHelper_(callback, int64_t, DataType::INT64);
Copy link
Contributor

Choose a reason for hiding this comment

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

如果只是遍历int类型的话,tiny的命名有些不太准确,建议使用如PhiForEachIntegerDataType之类的命名

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/fluid/framework/data_type.h 里面定义的tiny是保持一致的

} while (0)

_PhiForEachDataTypeTiny_(PhiVisitDataTypeCallbackTiny);
#undef PhiVisitDataTypeCallbackTiny
Copy link
Contributor

Choose a reason for hiding this comment

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

对于没有检测到的输入类型,最好还是抛出异常提示
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

已加入

"The first dim of the shape of Input(Step) should "
"be 1, but got %d",
step_dims[0]));
out->set_dims({-1});
Copy link
Contributor

Choose a reason for hiding this comment

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

dtype在这里可以设置吗?

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

Comment on lines +2102 to +2631
out->set_dims(out_dims);
if (return_inverse) {
index->set_dims(phi::make_ddim({x.dims()[axis_value]}));
}
}
if (return_index) {
indices->set_dims(phi::make_ddim({-1}));
}
if (return_counts) {
counts->set_dims(phi::make_ddim({-1}));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

这些dtype可以设置下

Copy link
Contributor Author

Choose a reason for hiding this comment

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

这些dtype会等到kernel执行的时候,执行VisitDataType函数进行确定,否则需要把那一套逻辑搬进来。

#include "paddle/phi/core/kernel_registry.h"
#include "paddle/phi/kernels/copy_kernel.h"
#include "paddle/phi/kernels/funcs/range_function.h"
#include "paddle/phi/kernels/range_kernel.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

range_kernel.h放在开头

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

#include "paddle/phi/backends/gpu/gpu_context.h"
#include "paddle/phi/core/kernel_registry.h"
#include "paddle/phi/kernels/impl/unstack_grad_kernel_impl.h"
#include "paddle/phi/kernels/unstack_grad_kernel.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

unstack_grad_kernel.h放在开头

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

#include "paddle/phi/backends/gpu/gpu_context.h"
#include "paddle/phi/core/kernel_registry.h"
#include "paddle/phi/kernels/impl/unstack_kernel_impl.h"
#include "paddle/phi/kernels/unstack_kernel.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

unstack_kernel.h放在开头

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

Comment on lines 19 to 21
KernelSignature StackOpArgumentMapping(const ArgumentMappingContext& ctx) {
return KernelSignature("stack", {"X"}, {"axis"}, {"Y"});
}
Copy link
Contributor

Choose a reason for hiding this comment

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

这个前向的ArgumentMapping比较常规,感觉使用默认的也能work,可以去掉试试

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 19 to 21
KernelSignature UnStackOpArgumentMapping(const ArgumentMappingContext& ctx) {
return KernelSignature("unstack", {"X"}, {"axis", "num"}, {"Y"});
}
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.

已删除

namespace phi {

KernelSignature UniqueOpArgumentMapping(const ArgumentMappingContext& ctx) {
return KernelSignature("unique_raw",
Copy link
Contributor

Choose a reason for hiding this comment

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

如果unique的单测覆盖率没通过的话,可以使用下面的方式选择下kernel

bool is_sorted = paddle::any_cast<bool>(ctx.Attr("is_sorted"));
if (is_sorted) {
 reutrn "unique"
} else {
 return "unique_raw"
}

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 link
Contributor

@YuanRisheng YuanRisheng left a comment

Choose a reason for hiding this comment

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

这几个算子有添加benchmark脚本吗

@@ -15,6 +15,7 @@ limitations under the License. */
#ifdef PADDLE_WITH_XPU
#include "paddle/fluid/operators/range_op.h"
#include "paddle/fluid/framework/op_registry.h"
#include "paddle/phi/kernels/funcs/math_function.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.

已删除

"The first dim of the shape of Input(Step) should "
"be 1, but got %d",
step_dims[0]));
out->set_dims({-1});
Copy link
Contributor

Choose a reason for hiding this comment

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

这个out是否需要设置一下dtype

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/phi/core/kernel_registry.h"
#include "paddle/phi/kernels/copy_kernel.h"
#include "paddle/phi/kernels/funcs/range_function.h"
#include "paddle/phi/kernels/range_kernel.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.

已修改

x.dims().size()));
auto out_dims = x.dims();
out_dims[axis_value] = -1;
out->set_dims(out_dims);
Copy link
Contributor

Choose a reason for hiding this comment

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

需要set_dtype一下吗

Copy link
Contributor Author

Choose a reason for hiding this comment

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

这些dtype会等到kernel执行的时候,执行VisitDataType函数进行确定,否则需要把那一套逻辑搬进来。

#pragma once
#include "paddle/phi/core/enforce.h"

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.

这里要加funcs的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.

已添加


#include "paddle/phi/kernels/funcs/for_range.h"

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.

同上

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

#include "paddle/phi/core/kernel_registry.h"
#include "paddle/phi/kernels/copy_kernel.h"
#include "paddle/phi/kernels/funcs/range_function.h"
#include "paddle/phi/kernels/range_kernel.h"
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.

done

@csy0225
Copy link
Contributor Author

csy0225 commented Mar 25, 2022

这几个算子有添加benchmark脚本吗

有添加

YuanRisheng
YuanRisheng previously approved these changes Mar 30, 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

@chenwhql chenwhql merged commit 74894cd into PaddlePaddle:develop Mar 31, 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