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

[opt] Eliminate useless local stores and atomics #858

Merged
merged 8 commits into from
Apr 24, 2020

Conversation

xumingkuan
Copy link
Contributor

@xumingkuan xumingkuan commented Apr 23, 2020

Related issue = #656

gather_used_atomics can be utilized in #857.

Benchmark:
before:
benchmark20200423

after: nothing changed.

after adding a test...:
benchmark20200423_3

[Click here for the format server]

@xumingkuan xumingkuan marked this pull request as draft April 23, 2020 22:08
@xumingkuan
Copy link
Contributor Author

Wait... there may be issues when it comes into IfStmt. It should fail in cases like this:

a = 1
if ...:
  a = 2
else:
  b = a

@xumingkuan xumingkuan changed the title [opt] Eliminate useless local stores [opt] Eliminate useless local stores and atomics Apr 24, 2020
@xumingkuan xumingkuan marked this pull request as ready for review April 24, 2020 00:53
@xumingkuan xumingkuan requested a review from yuanming-hu April 24, 2020 00:54
@xumingkuan xumingkuan mentioned this pull request Apr 24, 2020
18 tasks
@yuanming-hu
Copy link
Member

The logic here is getting too complex and the correctness is no longer trivial. Refactoring is needed here, otherwise, nobody other than you can understand what this part is doing...

Is seems that you are simulating a state machine here - maybe we should extract this part of logic.

Given we are rushing for the deadline next week, this can be merged for now if nothing breaks, but we should revisit later after the deadline.

@xumingkuan
Copy link
Contributor Author

The logic here is getting too complex and the correctness is no longer trivial. Refactoring is needed here, otherwise, nobody other than you can understand what this part is doing...

Is seems that you are simulating a state machine here - maybe we should extract this part of logic.

Given we are rushing for the deadline next week, this can be merged for now if nothing breaks, but we should revisit later after the deadline.

Yes... In order to eliminate $243 here in #853 (comment), I need to check whether a store can be eliminated even if it's not the last store. And I thought it's better to write the code for local stores rather than write it for global stores directly, so I wrote this. The logic is more complex than I thought.

@xumingkuan xumingkuan merged commit 50ad29a into taichi-dev:master Apr 24, 2020
@xumingkuan xumingkuan deleted the opt-local2 branch May 9, 2020 21:32
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

3 participants