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 deny param list to solve unuse param cannot found problem #36996

Merged
merged 4 commits into from
Nov 6, 2021

Conversation

thisjiang
Copy link
Contributor

PR types

Bug fixes

PR changes

APIs

Describe

解决batch_normtranspose等op存在无用输出参数,导致cinn_launch_op运行报错的问题。

@paddle-bot-old
Copy link

paddle-bot-old bot commented Nov 4, 2021

Thanks for your contribution!
Please wait for the result of CI firstly. See Paddle CI Manual for details.

for (auto* op : cluster) {
if (deny_param_list.count(op->Name())) {
const auto* desc = op->Op();
if (desc == nullptr) continue;
Copy link
Contributor

@wzzju wzzju Nov 5, 2021

Choose a reason for hiding this comment

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

Suggested change
if (desc == nullptr) continue;
这部分可能为空吗?如果没有可能就加PADDLE_ENFORCE吧。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

可能吧,Pass中其它地方也有类似判断

Comment on lines 59 to 66
static std::unordered_map<std::string, std::string> deny_param_list = {
{"batch_norm", "ReserveSpace"}};

bool OpHasInput(const OpDesc* op_desc, const std::string& param_name) {
const auto& inputs = op_desc->InputNames();
return std::find(inputs.cbegin(), inputs.cend(), param_name) != inputs.cend();
}

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
static std::unordered_map<std::string, std::string> deny_param_list = {
{"batch_norm", "ReserveSpace"}};
bool OpHasInput(const OpDesc* op_desc, const std::string& param_name) {
const auto& inputs = op_desc->InputNames();
return std::find(inputs.cbegin(), inputs.cend(), param_name) != inputs.cend();
}

if (desc == nullptr) continue;

auto deny_param_name = deny_param_list.at(op->Name());
if (OpHasInput(desc, deny_param_name)) {
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
if (OpHasInput(desc, deny_param_name)) {
if (desc->Inputs.count(deny_param_name)) {

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

@@ -299,17 +307,41 @@ void AddCinnOpToGraph(const GraphNodeSet& cluster,
framework::OpDesc cinn_op_desc;
cinn_op_desc.SetType(kCinnLaunchOp);
std::vector<std::string> input_names;

std::unordered_set<std::string> deny_var_list;
Copy link
Contributor

Choose a reason for hiding this comment

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

deny_var_list的逻辑封装成一个函数吧,返回类型为std::unordered_setstd::string 即可。

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 61 to 64
static std::unordered_map<std::string, std::string> deny_param_list = {
{"batch_norm", "ReserveSpace"},
{"transpose2", "XShape"},
{"reshape2", "XShape"}};
Copy link
Contributor

Choose a reason for hiding this comment

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

这个还是放在函数外面吧,声明本文件的静态常量对象(注意命名),和kDelim类似,放在匿名空间中就不用加static修饰。

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

@@ -56,6 +56,40 @@ namespace {
// & FLAGS_deny_cinn_ops.
constexpr char kDelim[] = ";";

std::unordered_set<std::string> GetDenyVarNamesList(
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
std::unordered_set<std::string> GetDenyVarNamesList(
std::unordered_set<std::string> GetDenyVarNames(

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

@@ -56,6 +56,64 @@ namespace {
// & FLAGS_deny_cinn_ops.
constexpr char kDelim[] = ";";

static const std::unordered_map<std::string, std::unordered_set<std::string>>
Copy link
Contributor

@wzzju wzzju Nov 5, 2021

Choose a reason for hiding this comment

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

在匿名空间中不用再加static了。

Copy link
Contributor

@wzzju wzzju left a comment

Choose a reason for hiding this comment

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

LGTM

@wzzju wzzju merged commit 1653f99 into PaddlePaddle:develop Nov 6, 2021
@thisjiang thisjiang deleted the add_deny_param_list branch November 6, 2021 08:01
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.

2 participants