Skip to content

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
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是保持一致的

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.

已加入

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
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函数进行确定,否则需要把那一套逻辑搬进来。

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

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

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
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
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.

已删除

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脚本吗

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.

已删除

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.

已设置

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.

已修改

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函数进行确定,否则需要把那一套逻辑搬进来。

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.

已添加

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

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脚本吗

有添加

@csy0225 csy0225 force-pushed the unstack_range_stack_unique branch from e046cf1 to 9f02a9c Compare March 25, 2022 09:59
@csy0225 csy0225 force-pushed the unstack_range_stack_unique branch from 9f02a9c to 6706557 Compare March 25, 2022 11:22
@csy0225 csy0225 force-pushed the unstack_range_stack_unique branch from 6706557 to 0cbf5ad Compare March 25, 2022 11:37
@csy0225 csy0225 force-pushed the unstack_range_stack_unique branch from 0cbf5ad to faf37cb Compare March 25, 2022 13:11
@csy0225 csy0225 force-pushed the unstack_range_stack_unique branch from faf37cb to e311a91 Compare March 26, 2022 02:15
@csy0225 csy0225 force-pushed the unstack_range_stack_unique branch from 4ddcf0a to 76a9da2 Compare March 28, 2022 03:56
@csy0225 csy0225 force-pushed the unstack_range_stack_unique branch from 76a9da2 to 9914950 Compare March 28, 2022 04:45
@csy0225 csy0225 force-pushed the unstack_range_stack_unique branch from 9914950 to 3d70ba2 Compare March 28, 2022 04:59
@csy0225 csy0225 force-pushed the unstack_range_stack_unique branch from ab64ec6 to 3510cb8 Compare March 29, 2022 15:28
YuanRisheng
YuanRisheng previously approved these changes Mar 30, 2022
@csy0225 csy0225 force-pushed the unstack_range_stack_unique branch from 1582a16 to 7ccdb10 Compare March 30, 2022 13:26
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

5 participants