-
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
[type] Local adder structure #2136
Conversation
some performance numbers comparing two kinds of loops:
|
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.
Awesome!! Just a few nits. Thanks!
: op_type(op_type), | ||
lhs(lhs), | ||
rhs(rhs), | ||
is_bit_vectorized(is_bit_vectorized) { |
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.
This is a little too much intrusion into the existing system. It seems to me that is_bit_vectorized
is only used in the bit_loop_vectorize
pass - maybe you can use an std::unordered_map<Stmt *, bool>
member variable in class BitLoopVectorize
, instead of adding a new field in class BinaryOpStmt
? (Just like llvm_val
in LLVM codegens.)
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_bit_vectorized
is used only in the bit_loop_ vectorize
pass when tagged on BinaryOpStmt
and this part should be replaced with some pass-scope data structure, just as you suggested. But for GlobalPtrStmt
and GetChStmt
, they need the tag to pass later passes including lower_access
and type_check
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.
Right, that's what I meant: we can use a pass-scope data structure just for BinaryOpStmt::is_bit_vectorized
. Given we are rushing for the deadline it's fine that we don't do it now.
and_b_c->is_bit_vectorized = true; | ||
// modify IR | ||
auto and_a_b_p = and_a_b.get(); | ||
stmt->insert_before_me(std::move(load_a)); |
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.
A more elegant way to do this:
VecStatement statements; |
Use VecStatement::push_back<...>
to create the statements and stmt->insert_before_me(vec_stmt)
to insert. (You don't need the DelayedIRModifier
part.)
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 find to do it the elegant way we may still need DelayedIRModifier
and there are other places that require changes as well(e.g. see the visitor for GlobalLoadStmt
), therefore I think we should do it in a separate PR later and refactoring all this together. This also applies to the changes for BinaryOpStmt::is_bit_vectorized
.
stmt->insert_before_me(std::move(load_c)); | ||
stmt->insert_before_me(std::move(carry_c)); | ||
stmt->insert_before_me(std::move(sum_c)); | ||
stmt->insert_before_me(std::move(load_b)); | ||
stmt->insert_before_me(std::move(carry_b)); | ||
stmt->insert_before_me(std::move(sum_b)); | ||
stmt->insert_before_me(std::move(sum_a)); |
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.
Ditto.
Related issue = #1905
[Click here for the format server]