-
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
[Opt] Constant folding for BitExtractStmt #1307
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1307 +/- ##
==========================================
+ Coverage 66.16% 66.68% +0.52%
==========================================
Files 36 36
Lines 5077 5094 +17
Branches 934 931 -3
==========================================
+ Hits 3359 3397 +38
+ Misses 1554 1539 -15
+ Partials 164 158 -6
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.
Thanks, LGTM in general, I only have nits.
taichi/transforms/constant_fold.cpp
Outdated
result = (input->val[0].val_int() >> stmt->bit_begin) & | ||
((1LL << (stmt->bit_end - stmt->bit_begin)) - 1); | ||
} else { | ||
result = (input->val[0].val_uint() >> stmt->bit_begin) & | ||
((1LL << (stmt->bit_end - stmt->bit_begin)) - 1); |
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.
I thought BitExtractStmt is only used for i32?
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.
We probably need to support unsigned inputs as well :-)
if (!input) | ||
return; |
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 (!input) | |
return; | |
if (!input || input->element_type() == DataType::i32) | |
return; |
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.
Why not do constant folding when the input
is i32
?
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! Just one nit. Thanks!
taichi/transforms/constant_fold.cpp
Outdated
result = (input->val[0].val_int() >> stmt->bit_begin) & | ||
((1LL << (stmt->bit_end - stmt->bit_begin)) - 1); | ||
} else { | ||
result = (input->val[0].val_uint() >> stmt->bit_begin) & | ||
((1LL << (stmt->bit_end - stmt->bit_begin)) - 1); |
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.
We probably need to support unsigned inputs as well :-)
taichi/transforms/constant_fold.cpp
Outdated
auto result_stmt = Stmt::make<ConstStmt>( | ||
LaneAttribute<TypedConstant>(TypedConstant(DataType::i32, result))); |
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.
Given the part before already uses uint
and int
which covers all integral types, here maybe we should support all integral types as well? Just change DataType::i32
into the input data type.
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.
Or we may need an assertion on input_type != DataType::i8 && input_type != DataType::i16
?
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.
Given the part before already uses
uint
andint
which covers all integral types, here maybe we should support all integral types as well? Just changeDataType::i32
into the input data type.
The typecheck
pass set ret_type = i32
for OffsetAndExtractBitsStmt
. Shall we also change it to the input data type?
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.
Or we may need an assertion on
input_type != DataType::i8 && input_type != DataType::i16
?
I think this is OK -- if the input is representable by i8
, so is the output.
* [Opt] Constant folding for BitExtractStmt * fix tests * Let BitExtractStmt support all integral types
Related issue = #656 #510 close #1278
[Click here for the format server]