-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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 schedule error #54983
Cinn schedule error #54983
Conversation
你的PR提交成功,感谢你对开源项目的贡献! |
25b4706
to
2d2ce5f
Compare
2d2ce5f
to
ba6204e
Compare
46c8e53
to
272c385
Compare
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 for std::cout
#define CINN_THROW(...) \ | ||
do { \ | ||
try { \ | ||
throw enforce::EnforceNotMet(__VA_ARGS__, __FILE__, __LINE__); \ |
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.
Is this same as Paddle
enforce? As I remember, we discussed offline that we use PADDLE_THROW
when we found the definition, otherwise we define by ourself, then we can handle it with both CINN-only and Paddle-CINN.
Does this implementation also handle both CINN-only and Paddle-CINN? Do we reuse Paddle code?
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.
PADDLE_THROW
is more complicated and has many redundant functions. If we want to reuse Paddle code we have to include more Paddle header files, thus we cannot build CINN-ONLY.
Using CINN_THROW
defined by ourselves maybe a good solution for now, 'cause there is IR_THROW
in PADDLE_IR
to deal with the similar problem.
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, not best but fine to me now.
paddle/cinn/ir/ir_schedule_error.h
Outdated
/** | ||
* \brief Indicates the level of printing error message in the current Schedule | ||
*/ | ||
enum class ScheduleErrorMessageLevel : int32_t { |
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.
Can the ErrorMessage use at other places? If so, should we remove Schedule
in naming? Then we can use the code you implemented all over the CINN!
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.
Technically no problem. But its namespace is cinn::ir
now, i don't know if it is convenient/necessary to use this in the same level of ir
, like hlir
, optim
, etc.
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.
What about changing the name and the location of this file when we have the need later?
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 so, can we change the namespace, location of file in this PR? We would like to reuse for other places.
@@ -160,6 +160,11 @@ DEFINE_int32(cinn_profiler_state, | |||
"Specify the ProfilerState by Int in CINN, 0 for kDisabled, 1 for " | |||
"kCPU, 2 for kCUDA, 3 for kAll, default 0."); | |||
|
|||
DEFINE_int32(cinn_schedule_error_message_level, |
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.
Same as my other comment, can the ErrorMessage be use at other places? If so, should we remove "schedule" text in the flag? Then we can use the code you implemented all over the CINN!
paddle/cinn/ir/ir_schedule_util.cc
Outdated
|
||
class InferFactorErrorHandler : public IRScheduleErrorHandler { | ||
public: | ||
explicit InferFactorErrorHandler(const ModuleExpr& module_expr) |
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.
Will it be complex if we have to implement sub-classes of IRScheduleErrorHandler
at where we want to report error?
I thought we have a ErrorHandler
class which can set general error message and detailed message, then we just input strings at where we want to report error.
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.
Two reasons:
- Different kinds of schedule primitive require different error messages, so using abstract class and sub-classes is to deal with their own special need for this.
- Implementation of error message inside each subclass is easier to use the macro to simplify the code.
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 for cout
Discussed offline, we approve this PR now, but above unresolved comments will be changed in the future PRs. |
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
* [CINN] Schedule error message optimization * format code style * add test * fix format * using CINN_THROW and using flags * optimize error msg * do not use abtract class of error hanlder * fix header
* [CINN] Schedule error message optimization * format code style * add test * fix format * using CINN_THROW and using flags * optimize error msg * do not use abtract class of error hanlder * fix header
PR types
Others
PR changes
Others
Description
新增
Schedule
报错处理机制card-72613
内容:
schedule primitive
通过继承基类IRScheduleErrorHandler
,完成不同的报错信息打印ScheduleImpl
中新增ScheduleErrorMessageLevel
,决定后续出现错误时的报错打印层级,可通过运行时的FLAGS控制Split
为例,展示如何在代码中使用新的报错处理机制CINN_THROW
,与主框架的PADDLE_THROW
报错信息内容保持一致效果: