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
[Bug] [opt] [doc] Fix OffsetAndExtractBitsStmt optimization and improve documentation on virtual/physical indices #1259
Conversation
taichi/ir/snode.cpp
Outdated
int SNode::get_num_bits(int virtual_index) const { | ||
int result = 0; | ||
const SNode *snode = this; | ||
auto physical_index = snode->physical_index_position[virtual_index]; | ||
while (snode) { | ||
result += extractors[physical_index].num_bits; | ||
result += snode->extractors[physical_index].num_bits; | ||
snode = snode->parent; | ||
} |
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 are two issues here:
- We should use virtual indices as input and do a translation to physical indices within the function (see the doc updates)
snode->extractors
instead ofextractors
:-)
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.
For the second point, sorry for writing a bug here :)
For the first point, I think we're already using physical indices in LoopIndexStmt
s. See
taichi/taichi/transforms/lower_ast.cpp
Lines 286 to 287 in ca81fdf
Stmt *loop_index = new_statements.push_back<LoopIndexStmt>( | |
new_for.get(), snode->physical_index_position[i]); |
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.
Ah you are right! I just now reverted that modification.
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 in general, but I'm not sure if get_num_bits
should take virtual_index
as input.
taichi/ir/snode.cpp
Outdated
int SNode::get_num_bits(int virtual_index) const { | ||
int result = 0; | ||
const SNode *snode = this; | ||
auto physical_index = snode->physical_index_position[virtual_index]; | ||
while (snode) { | ||
result += extractors[physical_index].num_bits; | ||
result += snode->extractors[physical_index].num_bits; | ||
snode = snode->parent; | ||
} |
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.
For the second point, sorry for writing a bug here :)
For the first point, I think we're already using physical indices in LoopIndexStmt
s. See
taichi/taichi/transforms/lower_ast.cpp
Lines 286 to 287 in ca81fdf
Stmt *loop_index = new_statements.push_back<LoopIndexStmt>( | |
new_for.get(), snode->physical_index_position[i]); |
Codecov Report
@@ Coverage Diff @@
## master #1259 +/- ##
==========================================
+ Coverage 84.78% 84.81% +0.02%
==========================================
Files 18 18
Lines 3267 3273 +6
Branches 613 615 +2
==========================================
+ Hits 2770 2776 +6
Misses 360 360
Partials 137 137
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.
Related issue = #1178 #1229
[Click here for the format server]