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

[XPU] Add embedding plugin #56488

Merged
merged 26 commits into from Aug 24, 2023
Merged

Conversation

csy0225
Copy link
Contributor

@csy0225 csy0225 commented Aug 21, 2023

PR types

New features

PR changes

Others

Description

Add embedding kernel plugin for small size dict.

@paddle-bot
Copy link

paddle-bot bot commented Aug 21, 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.

@csy0225 csy0225 force-pushed the seam_feat_opt branch 2 times, most recently from d1da251 to 3303311 Compare August 22, 2023 04:50
ym,
padding_idx);
#else
r = xpu::plugin::embedding_tiny_dict<XPUType, int64_t>(
Copy link
Contributor

Choose a reason for hiding this comment

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

统一下名称吧 fast_embedding 以后各种 case 的加速版在 fast_embedding 的wrapper 内部判断。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

感觉 fast 名称不好啊,没办法显示出 kernel 的具体场景,我这个是针对小词表下面的 kernel 优化,感觉这个名称更合适。一个 plugin 倒可以改成 fast,如果接下来有需要针对 embedding 其他场景进行优化,就不太好取名字了。

#endif
} else {
#ifndef PADDLE_WITH_XPU_PLUGIN
int64_t *ids_tt = RAII_GUARD.alloc_l3_or_gm<int64_t>(ids_t->numel());
Copy link
Contributor

Choose a reason for hiding this comment

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

xpu::embedding 不支持 int32 的 index 吗?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

这个是之前 phi kernel 的逻辑, kernel 里面会插入 cast 算子将 int32 转成 int64,昆仑2我看是支持int32的,昆仑一不清楚,所以为了兼容性,我没有改原来的逻辑

GET_IR_NODE(embedding);
GET_IR_NODE(embedding_ids);
auto* block = cast->Op()->Block();
auto cast_node_attr_in_dtype = cast->Op()->GetAttrIfExists<int>("in_dtype");
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. 有考虑 in_dtype 为 int32,out_dtype 为 int64 的情况吗?理论上这个情况下,可以把 cast 删除。
  2. 理论上不应该限制 in_dtype 吧?是不是只看 out_dtype==int64就行?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

这个在 cast + embedding 组合里面感觉应该不会出现,embedding 支持 int32,理论上不会出现你这个情况

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

@hong19860320 hong19860320 left a comment

Choose a reason for hiding this comment

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

LGTM

@zhupengyang zhupengyang merged commit 2a5adc5 into PaddlePaddle:develop Aug 24, 2023
26 checks passed
BeingGod pushed a commit to BeingGod/Paddle that referenced this pull request Sep 9, 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.

None yet

4 participants