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

add sin and cos optional parameters to fused_rope op #55415

Merged
merged 10 commits into from
Jul 26, 2023

Conversation

tianhaodongbd
Copy link
Contributor

@tianhaodongbd tianhaodongbd commented Jul 13, 2023

PR types

Others

PR changes

Others

Description

Pcard-70458
fused_rope算子功能加强,支持传入optional的sin、cos输入

@paddle-bot
Copy link

paddle-bot bot commented Jul 13, 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.

@tianhaodongbd tianhaodongbd reopened this Jul 17, 2023
@tianhaodongbd tianhaodongbd changed the title modified fused_rope forward op add sin cos to fused_rope op Jul 17, 2023
@tianhaodongbd tianhaodongbd changed the title add sin cos to fused_rope op add sin and cos optional parameters to fused_rope op Jul 17, 2023
@@ -0,0 +1,2 @@
timeout: invalid option -- '1'
Try 'timeout --help' for more information.
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.

收到 已删除

@@ -124,6 +124,68 @@ def get_forward_backward(self, rope_function, seed):

return fw, bw

def get_forward_backward_sin_cos(self, rope_function, seed):
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.

已修改


for (; index < size; index += stride) {
#pragma unroll
for (int nx = 0; nx < VecSize; ++nx) {
Copy link
Contributor

Choose a reason for hiding this comment

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

  • VectorizedFusedRopeWithSinCosKernelVectorizedFusedRopeKernel只有L46-L56、L108-L117的实现不同,其他代码都一样,没有必要定义成两个函数,代码尽量复用一下。
  • sin、cos计算的逻辑,算子前、反向也是一样的,可以挪到头文件中以复用

Copy link
Contributor Author

Choose a reason for hiding this comment

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

已修改

…& move VectorizedFusedRopeKernel to fused_rope_utils.h
Xreki
Xreki previously approved these changes Jul 24, 2023
Copy link
Contributor

@Xreki Xreki left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@qingqing01 qingqing01 left a comment

Choose a reason for hiding this comment

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

LGTM for the changes of Inputs/Output/Attrs of OPs

Copy link
Contributor

@sunzhongkai588 sunzhongkai588 left a comment

Choose a reason for hiding this comment

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

LGTM for docs

  • 补充一下中文文档吧

@AnnaTrainingG AnnaTrainingG merged commit 581d05b into PaddlePaddle:develop Jul 26, 2023
wz1qqx pushed a commit to wz1qqx/Paddle that referenced this pull request Jul 31, 2023
jinjidejinmuyan pushed a commit to jinjidejinmuyan/Paddle that referenced this pull request Aug 30, 2023
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