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

[IR]add kernel dialect #54428

Merged
merged 4 commits into from
Jun 8, 2023
Merged

Conversation

phlrain
Copy link
Collaborator

@phlrain phlrain commented Jun 7, 2023

PR types

Others

PR changes

Others

Description

新增kernel dialect

Other

Pcard-67164

@paddle-bot
Copy link

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

void PhiKernelOp::Verify(const std::vector<ir::OpResult> &inputs,
const std::vector<ir::Type> &outputs,
const ir::AttributeMap &attributes) {
VLOG(4) << "Verifying inputs, outputs and attributes for: SetParameterOp.";
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
VLOG(4) << "Verifying inputs, outputs and attributes for: SetParameterOp.";
VLOG(4) << "Verifying inputs, outputs and attributes for: PhiKernelOp.";


// Verify if attributes contain attribute name in attributes_name:
// if (!attributes.at("parameter_name").isa<StrAttribute>()) {
// throw("Type of attribute: parameter_name is not right.");
Copy link
Contributor

Choose a reason for hiding this comment

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

这里是默认不进行任何Verify 操作么,期望行为就是这样的么?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

这个是需要在开发中,往内部添加数据,然后统一完善verify

AllocatedDenseTensorTypeStorage);

static AllocatedDenseTensorType get(ir::IrContext *ctx,
phi::Place place,
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
phi::Place place,
const phi::Place& place,

static AllocatedDenseTensorType get(ir::IrContext *ctx,
phi::Place place,
ir::Type dtype,
phi::DDim dims,
Copy link
Contributor

Choose a reason for hiding this comment

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

同上,这里的参数有些应该是const & 的?另外,参数的顺序是否有偏好要求,这个看起来是一个底层数据Type。

namespace paddle {
namespace dialect {

class PaddleKernelDialect : public ir::Dialect {
Copy link
Contributor

Choose a reason for hiding this comment

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

我看下面有些数据结构命名是以Phi开头,有些是以Paddle开头,我们的区分原则是什么,还是说「统一」成一个词也没有问题?

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 自己是一个不在paddle下的namespace; 这个确实可以统一下,都用同一个的单词

@@ -0,0 +1,62 @@
// Copyright (c) 2023 PaddlePaddle Authors. All Rights Reserved.
Copy link
Contributor

Choose a reason for hiding this comment

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

pd_kernel_dialect 要和已经存在 pd_dialect 都铺在一个目录下么?是否需要分成两个目录?

@@ -112,6 +112,15 @@ struct DenseTensorTypeStorage : public ir::TypeStorage {
return ParamKey(dtype_, dims_, layout_, lod_, offset_) == key;
}

bool operator==(const DenseTensorTypeStorage &storage) const {
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
Collaborator Author

Choose a reason for hiding this comment

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

这个是AllocatedTensorTypeStrorage 依赖了 DenseTensorTypeStorage ,我一起删了

Copy link
Contributor

@zhangbo9674 zhangbo9674 left a comment

Choose a reason for hiding this comment

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

LGTM

@phlrain phlrain merged commit 950a29b into PaddlePaddle:develop Jun 8, 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.

4 participants