Skip to content
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

[async] Support global temporaries value state #2061

Merged
merged 3 commits into from
Nov 24, 2020

Conversation

k-ye
Copy link
Member

@k-ye k-ye commented Nov 23, 2020

I've extended AsyncNode::snode to hold a std::variant<SNode*, Kernel*>, because global temporaries can be identified by its enclosing kernel. (I feel like this is more intuitive than having a global_tmp in the enum class. Otherwise, what should the SNode be?)

Related issue = #2024

[Click here for the format server]


@@ -119,18 +143,33 @@ TaskMeta *get_task_meta(IRBank *ir_bank, const TaskLaunchRecord &t) {

// TODO: this is an abuse since it gathers nothing...
gather_statements(root_stmt, [&](Stmt *stmt) {
if (auto global_load = stmt->cast<GlobalLoadStmt>()) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the real change. The rest is just to adapt to snode_or_global_tmp..

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool -- We may use a snode for global temps, merge GlobalTemporaryStmt into GlobalPtrStmt and handle this in get_meta_input_value_states() (which calls ControlFlowGraph::gather_loaded_snodes()) in the future.

Copy link
Contributor

@xumingkuan xumingkuan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool!

@@ -119,18 +143,33 @@ TaskMeta *get_task_meta(IRBank *ir_bank, const TaskLaunchRecord &t) {

// TODO: this is an abuse since it gathers nothing...
gather_statements(root_stmt, [&](Stmt *stmt) {
if (auto global_load = stmt->cast<GlobalLoadStmt>()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool -- We may use a snode for global temps, merge GlobalTemporaryStmt into GlobalPtrStmt and handle this in get_meta_input_value_states() (which calls ControlFlowGraph::gather_loaded_snodes()) in the future.

Comment on lines +311 to +314
if (state.type == AsyncState::Type::value && state.holds_snode()) {
const auto *sn = state.snode();
if (meta.element_wise.find(sn) == meta.element_wise.end() ||
!meta.element_wise[sn]) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should also apply to global temps because we never completely overwriting the state.

Maybe we should directly write sth like this in the above gather_statements, whose correctness is clearer to me:

if (stmt->is<GlobalTemporaryStmt>()) {
  meta.input_states.insert(AsyncState::for_global_tmp(t.kernel));
  meta.output_states.insert(AsyncState::for_global_tmp(t.kernel));
}

This may cause some tasks only reading the global temp buffer not swappable, but I think there won't be >1 consecutive tasks reading the buffer without writing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Marking GlobalTemporaryStmt whenever we see it, as both input and output states sounds good to me!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#2061 (comment)

Ha, that's actually where I started (locally)... Done

Copy link
Member

@yuanming-hu yuanming-hu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! LGTM!

Comment on lines +311 to +314
if (state.type == AsyncState::Type::value && state.holds_snode()) {
const auto *sn = state.snode();
if (meta.element_wise.find(sn) == meta.element_wise.end() ||
!meta.element_wise[sn]) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Marking GlobalTemporaryStmt whenever we see it, as both input and output states sounds good to me!

@codecov
Copy link

codecov bot commented Nov 24, 2020

Codecov Report

Merging #2061 (cc7c90d) into master (28d312b) will increase coverage by 0.95%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2061      +/-   ##
==========================================
+ Coverage   42.60%   43.56%   +0.95%     
==========================================
  Files          45       45              
  Lines        6478     6267     -211     
  Branches     1110     1110              
==========================================
- Hits         2760     2730      -30     
+ Misses       3546     3366     -180     
+ Partials      172      171       -1     
Impacted Files Coverage Δ
python/taichi/lang/ast_checker.py 70.58% <0.00%> (-1.64%) ⬇️
python/taichi/testing.py 75.00% <0.00%> (-0.72%) ⬇️
python/taichi/lang/linalg.py 89.33% <0.00%> (-0.67%) ⬇️
python/taichi/lang/__init__.py 40.41% <0.00%> (-0.24%) ⬇️
python/taichi/misc/util.py 20.32% <0.00%> (-0.21%) ⬇️
python/taichi/misc/task.py 0.00% <0.00%> (ø)
python/taichi/tools/patterns.py 0.00% <0.00%> (ø)
python/taichi/lang/kernel.py 73.00% <0.00%> (+0.16%) ⬆️
python/taichi/misc/gui.py 8.89% <0.00%> (+0.18%) ⬆️
python/taichi/main.py 22.95% <0.00%> (+0.35%) ⬆️
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 28d312b...cc7c90d. Read the comment docs.

@yuanming-hu yuanming-hu merged commit 0b8439f into taichi-dev:master Nov 24, 2020
@yuanming-hu yuanming-hu mentioned this pull request Nov 30, 2020
@k-ye k-ye deleted the glb-tmp-state branch December 20, 2020 14:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants