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 SFG-level DSE for scalar SNodes #1907

Merged
merged 13 commits into from
Oct 2, 2020
Merged

Conversation

k-ye
Copy link
Member

@k-ye k-ye commented Sep 29, 2020

Sorry for this large change. It shouldn't be as complicated as the size implies.. The approach is basically to find out those scalar SNodes whose values are not read by the successor tasks. We then pass in these SNodes to the CFG pass, and instruct the live variable analysis not to add them into the final node's live_gen.

There are a few special handlings for scalar SNodes here:

  1. Upon seeing a GlobalStoreStmt when constructing the task meta, we add the SNode to the input (reader) states, iff. it is not a scalar.
    if (auto global_store = stmt->cast<GlobalStoreStmt>()) {
    if (auto ptr = global_store->ptr->cast<GlobalPtrStmt>()) {
    for (auto &snode : ptr->snodes.data) {
    meta.input_states.emplace(snode, AsyncState::Type::value);
  2. In SFG::optimize_dead_store(), if the state forms a flow edge, we also check if it's a scalar and if the successor task actually reads it.
    if (task->has_state_flow(s, other)) {
    used = true;
  3. atomic demotion for global values are only enabled for serial tasks. I also made them handle scalars as well.
    } else if (!is_parallel_executed) {

Finally, the optimization result is cached, much like what @xumingkuan has done in #1889.

Related issue = #742

[Click here for the format server]


@k-ye k-ye removed the skip ci label Sep 30, 2020
@k-ye k-ye marked this pull request as ready for review September 30, 2020 11:51
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.

Thank you! I would leave the control_flow_graph part to @xumingkuan but everything else LGTM!

tests/python/test_sfg.py Outdated Show resolved Hide resolved
stats = ti.get_kernel_stats()
stats.clear()

for _ in range(5):
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
for _ in range(5):
for _ in range(2):

Does this make the test stricter?

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually the DSE works pretty deterministically :) I just wasn't sure if it was a good idea to be very specific about how many DSE has been done..

taichi/ir/control_flow_graph.cpp Outdated Show resolved Hide resolved
taichi/program/ir_bank.h Outdated Show resolved Hide resolved
k-ye and others added 5 commits October 1, 2020 18:29
Co-authored-by: Yuanming Hu <yuanming-hu@users.noreply.github.com>
Co-authored-by: Yuanming Hu <yuanming-hu@users.noreply.github.com>
Co-authored-by: Yuanming Hu <yuanming-hu@users.noreply.github.com>
@k-ye k-ye added the skip ci label Oct 1, 2020
@codecov
Copy link

codecov bot commented Oct 1, 2020

Codecov Report

Merging #1907 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1907   +/-   ##
=======================================
  Coverage   43.83%   43.83%           
=======================================
  Files          45       45           
  Lines        6194     6194           
  Branches     1100     1100           
=======================================
  Hits         2715     2715           
  Misses       3310     3310           
  Partials      169      169           

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 21ad346...670c95c. Read the comment docs.

@k-ye k-ye force-pushed the graph-dse branch 2 times, most recently from 34c56a7 to 2989b55 Compare October 1, 2020 14:17
Copy link
Collaborator

@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.

The control_flow_graph part LGTM!

@k-ye k-ye force-pushed the graph-dse branch 2 times, most recently from d8ea33d to 51f9219 Compare October 1, 2020 21:50
@k-ye k-ye merged commit a1c6d7c into taichi-dev:master Oct 2, 2020
@k-ye k-ye deleted the graph-dse branch October 2, 2020 10:10
@yuanming-hu yuanming-hu mentioned this pull request Oct 3, 2020
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.

None yet

4 participants