-
Notifications
You must be signed in to change notification settings - Fork 5.9k
[CINN][New Hardware Update] Modify Target If-Else #64587
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][New Hardware Update] Modify Target If-Else #64587
Conversation
|
你的PR提交成功,感谢你对开源项目的贡献! |
| }; | ||
| target.arch.Match( | ||
| [&](common::NVGPUArch) { | ||
| if (!FLAGS_cinn_enable_map_expr && !FLAGS_cinn_new_group_scheduler) { |
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.
可以尝试提炼这些代码到单独的闭包里,比如叫NvGpuArchCompute。这样缩进更好看些。
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.
OK
| sch->Bind(rb_loops.back(), "threadIdx.x"); | ||
| sch->SetBuffer(rf_block, "local"); | ||
| }, | ||
| [&](std::variant<common::UnknownArch, common::X86Arch, common::ARMArch>) { |
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.
在函数体里显式写注释:// Do nothing.
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.
OK
| bytes, | ||
| cudaMemcpyDeviceToDevice, | ||
| static_cast<cudaStream_t>(stream)); | ||
| return true; |
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.
这些语义没有无脑地维持,虽然实质上差不多。
本函数的重构应该这样弄:
return input_target.arch.Match(
[&](...) {
...
return true;
}
)每个case提供一个返回值。
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.
可以的。不过如果原有代码只在部分case下有返回值,会难以处理。
| [&](common::X86Arch) { | ||
| std::copy(tensor->data<T>(), tensor->data<T>() + size, data.begin()); | ||
| }, | ||
| [&](std::variant<common::UnknownArch, common::ARMArch>) { |
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.
【只是建议】可以不可以不用std::variant做case?总感觉那样的代码层次更少更清爽。如果觉得有些地方会导致代码冗余,我们总是可以借助lambda提炼公共代码。
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.
感觉可读性差不多,std::variant更强调几种case执行共性操作
PR Category
CINN
PR Types
Improvements
Description
使用target.arch.Match替换DefaultNVGPUTarget分支。
pcard-79890