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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants