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

[CINN]fix symbol arg binding in bc optimize #70193

Merged
merged 3 commits into from
Dec 16, 2024

Conversation

Hongqing-work
Copy link
Contributor

PR Category

CINN

PR Types

Bug fixes

Description

Pcard-67164
This PR fixes symbol arg binding in bc optimize.

Copy link

paddle-bot bot commented Dec 13, 2024

你的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.

origin_symbol_args.at(arg_idx);
} else {
new_args_vec.emplace_back(func_args[arg_idx]);
break;
Copy link
Contributor

Choose a reason for hiding this comment

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

为什么是跳过var?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

因为当前func_arg是按照(tensor_arg1, tensor_arg2, .. tensor_argn, ... var_arg1, var_arg2, .. var_argn)组织的,这一步只收集TensorArg,var_arg(也就是SymbolArg)后面根据原始shape_or_data生成

Copy link
Contributor

Choose a reason for hiding this comment

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

那是不写成tensor_arg的判断代码更容易理解些?

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

std::vector<ir::Argument> new_args_vec;
int total_args_num = 0;
int cur_arg_idx = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

cur_arg_idx加点注释说明

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,直接改造其构造方法并放入AddSymbolArgs函数中,语义已经明确

@@ -136,7 +136,8 @@ CollectSubstituteDimExprMap(
[&](const symbol::DimExpr& dim_expr) {
if (dim_expr.isa<std::string>()) return false;
for (const auto& symbol : symbol::CollectDimExprSymbols(dim_expr)) {
if (new_symbol_set.count(symbol) == 0) {
if (new_symbol_set.count(symbol) == 0 &&
base_dim_expr_set.count(symbol) == 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

什么样的case会同时命中这两个条件?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

这段逻辑是想说如果被替换之后出现了new_symbol_set或者base_dim_expr_set里都没有的符号,那么它就是一个不能被子集替换的,很多case都有,比如BC(S0, S1),new_symbol_set里没有,输入符号里也没有S0和S1,那么它就can't BeRepresentedBySubset,需要被替换成新符号

@Hongqing-work Hongqing-work merged commit 0e62f85 into PaddlePaddle:develop Dec 16, 2024
28 checks passed
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