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

[NewIR]Change feed list to variable list && support GPU #55401

Merged

Conversation

phlrain
Copy link
Collaborator

@phlrain phlrain commented Jul 13, 2023

PR types

Bug fixes

PR changes

Others

Description

本pr有两个大的升级,

  1. 将feedlist换成一个单个的variable,方便share variable
  2. 引入shaddow feed kernel,在GPU场景,在feed 和后继op之间做数据copy

Others

Pcard-67164

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

@paddle-bot
Copy link

paddle-bot bot commented Jul 13, 2023

❌ The PR is not created using PR's template. You can refer to this Demo.
Please use PR's template, it helps save our maintainers' time so that more developers get helped.

if (index >= feed_inputs.size()) {
feed_inputs.resize(index + 1);
if (FLAGS_enable_new_ir_in_executor) {
VLOG(3) << "SetFeedVariable name=" << var_name << " index=" << index;
Copy link
Contributor

Choose a reason for hiding this comment

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

可以把这行log放到if外,else里的log就可以删了

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

namespace phi {

template <typename T, typename Context>
void FeedWithPlaceKernel(const Context& ctx,
Copy link
Contributor

Choose a reason for hiding this comment

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

既然有了shadow feed,那feedwithplace是啥作用

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

这个feed with place是跟动转静场景用的, 动转静的时候,tensor的place是已知的,需要将这个place 提供给AOT pass

namespace phi {

template <typename T, typename Context>
void ShaddowFeedKernel(const Context& ctx,
Copy link
Contributor

Choose a reason for hiding this comment

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

ShadowFeedKernel?或者直接改叫FeedKernel?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

是可以改的,但是改了和op 的名称对不上,还得做一层的映射

Copy link
Contributor

Choose a reason for hiding this comment

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

这里说的op名称是指的pd_op.yaml配置的名称吗,那个是可以改的吧,主要是现在这个名字拼写错误,看着有点难受

@phlrain phlrain changed the title Change feed list to variable list [NewIR]Change feed list to variable list && support GPU Jul 20, 2023
@@ -227,3 +227,30 @@
inplace: null
view: null
backward: null


- name: shaddow_feed
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- name: shaddow_feed
- name: shadow_feed

我查了下,应该是shadow?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

我改一下

phi::TransToPhiPlace(shaddow_key.backend()),
(*it)->result(0).type().dyn_cast<dialect::DenseTensorType>());

ir::Operation* shaddow_op =
Copy link
Contributor

Choose a reason for hiding this comment

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

从这里代码看来,我们还是希望有类似 ir::shadow(xxx) C++端API的,这样能简洁很多代码

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

是的


namespace paddle {
namespace dialect {

std::unique_ptr<ir::Program> PdOpLowerToKernelPass(ir::Program* prog);
std::unique_ptr<ir::Program> PdOpLowerToKernelPass(
ir::Program* prog, phi::Place place = phi::CPUPlace());
Copy link
Contributor

Choose a reason for hiding this comment

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

这里是不是可以不加默认的Place值?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

是需要的,我后续调整下

PD_REGISTER_KERNEL(feed_with_place,

PD_REGISTER_KERNEL(
feed_with_place, CPU, ALL_LAYOUT, phi::FeedWithPlaceKernel, float) {}
Copy link
Contributor

Choose a reason for hiding this comment

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

原来feed_with_place 注册了4种类型,这里为什么只保留了1种?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

当前的验证,只用到了float,是需要补全的

ctx.template Alloc<T>(out);
if (x.place() == out->place()) {
out->ShareDataWith(x);
out->set_lod(x.lod());
Copy link
Contributor

Choose a reason for hiding this comment

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

这个OP自带inplace策略是吧?属于kernel内inplace

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

是的,这个是一个特殊的op

Copy link
Contributor

@Aurelius84 Aurelius84 left a comment

Choose a reason for hiding this comment

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

LGTM overall

@phlrain phlrain merged commit 7551784 into PaddlePaddle:develop Jul 20, 2023
cqulilujia pushed a commit to cqulilujia/Paddle that referenced this pull request Jul 24, 2023
…#55401)

* add feed with place op

* remove useless unitest

* udpate mkldnn

* update

* new ir support builtin slice op

* fix phi kernel adaptor bug

* add enable_static

* remove useless test case

* change feed list to single variable

* support gpu

* fix bug

* remove template

* add more data type

* fix cimpile bug
wz1qqx pushed a commit to wz1qqx/Paddle that referenced this pull request Jul 31, 2023
…#55401)

* add feed with place op

* remove useless unitest

* udpate mkldnn

* update

* new ir support builtin slice op

* fix phi kernel adaptor bug

* add enable_static

* remove useless test case

* change feed list to single variable

* support gpu

* fix bug

* remove template

* add more data type

* fix cimpile bug
jinjidejinmuyan pushed a commit to jinjidejinmuyan/Paddle that referenced this pull request Aug 30, 2023
…#55401)

* add feed with place op

* remove useless unitest

* udpate mkldnn

* update

* new ir support builtin slice op

* fix phi kernel adaptor bug

* add enable_static

* remove useless test case

* change feed list to single variable

* support gpu

* fix bug

* remove template

* add more data type

* fix cimpile bug
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.

3 participants