| CARVIEW |
Navigation Menu
-
Notifications
You must be signed in to change notification settings - Fork 5.8k
New ir support combine op #54682
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
New ir support combine op #54682
Conversation
… add_kernel_dialect
… add_kernel_dialect
… lower_pd_op_to_kernel_dialect
… lower_pd_op_to_kernel_dialect
… lower_pd_op_to_kernel_dialect
… lower_pd_op_to_kernel_dialect
… new_interprector_support_new_IR
… new_interprector_support_new_IR
… new_interprector_support_new_IR
… new_interprector_support_new_IR
… new_interprector_support_new_IR
…e/Paddle into new_ir_support_combine_op
… new_ir_support_combine_op
|
你的PR提交成功，感谢你对开源项目的贡献! |
|
❌ The PR is not created using PR's template. You can refer to this Demo. |
… new_ir_support_combine_op
… new_ir_support_combine_op
… new_ir_support_combine_op
… new_ir_support_combine_op
… new_ir_support_combine_op
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
|
||
| #include <codecvt> | ||
| #include <iostream> | ||
| #include <locale> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
有的头文件看起来没用到
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
| @@ -70,6 +72,37 @@ void BuildScope(ir::Block* block, | |||
| continue; | |||
| } | |||
|
|
|||
| if (op_name == "builtin.combine") { | |||
| // fetch is a very special op, with no output | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这个注释可以删了
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
| auto op_func_node = const_cast<OpFuncNode*>((instr_node.OpFunc())); | ||
| VLOG(4) << op_func_node->phi_op_name_; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
建议VLOG的文案可以参考ENFORCE规范一下~~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这个代码merge develop后没有了。
| input_map[t.name] = index++; | ||
| // only suppurt non vector input for now | ||
| std::map<std::string, int> input_map; | ||
| int index = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这里的对 vector<OpInput/Output/Info> 的解析逻辑是不是可以抽离为单独的函数以复用？统一放到fluid/ir/dialect/utils.h 类似的地方？
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这块的逻辑，和下个comment，我在另外一个pr 重构中，这块的逻辑已经有点混乱了
| phi::DataType kernel_data_type = phi::DataType::UNDEFINED; | ||
| auto attr_map = op->attributes(); | ||
| auto data_type_info = runtime_info.kernel_key_dtype; | ||
| if (data_type_info.size() > 0 && data_type_info[0] != "") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
data_type_info[0] = “” 这个是什么场景下的？
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这个地方 可能是一个过度的保护，我尝试删一下
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这个删不掉，我们的模块传递有点问题，得专门解决下
| auto op_info_res = op_info_interface.GetOpInfo(); | ||
| auto runtime_info = std::get<3>(op_info_res); | ||
| std::string kernel_fn_str; | ||
| if (op_info_interface) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这里如果没有命中if分支，kernel_fn_str 默认是空，所以这里是否需要加一个else分支 PADDLE_THROW 下？
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
是存在部分的算子，没有注册op_info_interface的（这个是机制上允许的），需要对于这些算子做不同的处理
| @@ -70,6 +72,37 @@ void BuildScope(ir::Block* block, | |||
| continue; | |||
| } | |||
|
|
|||
| if (op_name == "builtin.combine") { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这里关于op_name的特判处理分支（以及上面的分支）可否抽离成一个单独的函数，假设为bool HandleSpecialOp(...)，类似如下：
if(HandleSpecialOp(op_name, xxx)){
continue;
}好处在于：
BuildScope函数里的逻辑很容易看懂主体逻辑- 后续有其他特判的分支，也不影响可读性，扩充或新增类似
HandleSpecialOp函数即可
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这块的逻辑，包含上一个pd_op_to_kernel_pass，我在另外一个pr 重构中，这块的逻辑已经有点混乱了
… new_ir_support_combine_op
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
PR types
Others
PR changes
Others
Description
支持combine op，在新的IR设计中，对于tensor list 做为输入的情况下，通过combine op转成一个value，在执行器做变量的share，降低执行的开销
Other
Pcard-67164