-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
[ir] Remove the usage of kernel from type_check() pass #1848
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1848 +/- ##
==========================================
+ Coverage 64.23% 64.26% +0.02%
==========================================
Files 19 19
Lines 3722 3725 +3
Branches 705 705
==========================================
+ Hits 2391 2394 +3
Misses 1204 1204
Partials 127 127
Continue to review full report at Codecov.
|
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 + nits!
@@ -34,9 +34,11 @@ class ConstantFold : public BasicStmtVisitor { | |||
} |
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.
Off-topic: lines 10-11 are redundant in this file.
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 (removed + reordered)
taichi/python/export_lang.cpp
Outdated
@@ -346,8 +346,9 @@ void export_lang(py::module &m) { | |||
current_ast_builder().insert(Stmt::make<FrontendBreakStmt>()); | |||
}); | |||
|
|||
m.def("create_kernel_return", [&](const Expr &value) { | |||
current_ast_builder().insert(Stmt::make<FrontendKernelReturnStmt>(value)); | |||
m.def("create_kernel_return", [&](const Expr &value, DataType dt) { |
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.
nit: Shall we use DataType dt
or const DataType &
(which is used in make_arg_load_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.
Ack, switched to const DataType&
(I tried to use DataType
for make_arg_load_expr
, but bumped into funny rvalue reference issues..)
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.
Cool LGTM!
Because we know the kernel args and return types when constructing these arg load/kernel return stmts, we don't have to re-infer them inside
type_check()
. This PR removes the necessity of kernel intype_check
(config is still not handled), so that IR is a bit more decoupled from Kernel.Related issue = #689
[Click here for the format server]