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] Dive into container statements to find local loads/stores for optimization, and optimize loads of new allocas to 0 #662

Merged
merged 12 commits into from Mar 27, 2020

Conversation

xumingkuan
Copy link
Collaborator

@xumingkuan
Copy link
Collaborator Author

I don't see a trivial way to check the statements for each kind of container statement. Shall we add a function like virtual std::vector<Stmt *> get_contained_statements() in addition to virtual bool is_container_statement()?

taichi/transforms/simplify.cpp Outdated Show resolved Hide resolved
taichi/transforms/simplify.cpp Outdated Show resolved Hide resolved
@yuanming-hu
Copy link
Member

I don't see a trivial way to check the statements for each kind of container statement. Shall we add a function like virtual std::vector<Stmt *> get_contained_statements() in addition to virtual bool is_container_statement()?

This can be a very useful analysis pass! Note that we use the visitor pattern to avoid adding too many methods in each statement. You can declare a class class GetContainedStatements : public IRVisitor and implement the contained statements under void visit(XStmt * stmt) for each container statement (e.g. body for WhileStmt).

@yuanming-hu
Copy link
Member

(Maybe I reviewed too early - feel free to re-request my review when you need next time.)

@xumingkuan
Copy link
Collaborator Author

I found BasicStmtVisitor already dives into container statements. I think this looks much clearer now, ignoring if (!advanced_optimization) clauses :)

The number of statements of test_ad_if_mutable reduced from 92 to 70.

@yuanming-hu
Copy link
Member

The number of statements of test_ad_if_mutable reduced from 92 to 70.

AWESOME!!

@xumingkuan xumingkuan changed the title [opt] Dive into container statements to find local loads/stores for optimization [Opt] Dive into container statements to find local loads/stores for optimization, and optimize loads of new allocas to 0 Mar 26, 2020
@xumingkuan xumingkuan marked this pull request as ready for review March 26, 2020 21:55
@xumingkuan
Copy link
Collaborator Author

xumingkuan commented Mar 26, 2020

Diving into container statements does nothing to test_ad_if_mutable, but optimizing

$1 = alloca
$2 = local load [ [$1[0]]]

to

$1 = alloca
$2 = const [0]

further reduces the number of statements to 58.

@yuanming-hu
Copy link
Member

Cool! At some point in the future, you may want to expand your test set and write an automatic tool to summarize how many instructions are generated in each test case :-)

@xumingkuan xumingkuan mentioned this pull request Mar 26, 2020
18 tasks
taichi/transforms/simplify.cpp Show resolved Hide resolved
#include "taichi/ir/ir.h"

TLANG_NAMESPACE_BEGIN

// Find if there is a load following a store in a basic block
Copy link
Member

Choose a reason for hiding this comment

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

It seems to me that LocalLoadSearcher is just finding loads, since LocalStore is not mentioned here. I'm confused...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh yes, LocalLoadSearcher is finding loads all over the root, with root actually be a statement after LocalStore, passed in as an argument. I'd better change the comment to simply Find if there is a load.

}
};

// Find if there is a store preceding a load in a basic block
Copy link
Member

Choose a reason for hiding this comment

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

Again, isn't this simply Find if there is a store?

}
};

// Find the **last** store preceding a load in a basic block
Copy link
Member

Choose a reason for hiding this comment

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

Again, how does this ensure preceding?

@xumingkuan
Copy link
Collaborator Author

Oh no I'm breaking tests. Go debugging.

@xumingkuan
Copy link
Collaborator Author

xumingkuan commented Mar 26, 2020

I see what the bug is: sometimes I find a store that is not the last one in LocalStoreForwarder.
There is one thing I want to make sure: can we optimize

$30 = atomic add($1, $8)
$31 = local load [ [$1[0]]]

to (replace all usages of $31 with $30)? Or does AtomicOpStmt provide a return value? And what about $31 = local load [ [$1[1]]]?

@yuanming-hu
Copy link
Member

Yes, AtomicOpStmt returns the old value

@xumingkuan
Copy link
Collaborator Author

Yes, AtomicOpStmt returns the old value

I see. So if the last statement is an AtomicOpStmt, there's no way we can directly optimize local load.

@xumingkuan
Copy link
Collaborator Author

Should be working now. The good news is number 58 here doesn't change.

Diving into container statements does nothing to test_ad_if_mutable, but optimizing

$1 = alloca
$2 = local load [ [$1[0]]]

to

$1 = alloca
$2 = const [0]

further reduces the number of statements to 58.

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 job!

taichi/transforms/simplify.cpp Outdated Show resolved Hide resolved
taichi/transforms/simplify.cpp Show resolved Hide resolved
@yuanming-hu yuanming-hu merged commit d6c96a8 into taichi-dev:master Mar 27, 2020
@xumingkuan xumingkuan deleted the load branch March 27, 2020 18:46
archibate pushed a commit to archibate/taichi that referenced this pull request Mar 31, 2020
…ptimization, and optimize loads of new allocas to 0 (taichi-dev#662)

* refactor BasicBlockSimplify::visit(LocalStoreStmt*)

* refactor again, making use of visitors

* [skip ci] same for visit(LocalLoadStmt *)

* [skip ci] LocalStoreForwarder

* Optimize loads of new allocas to const[0]

* [skip ci] enforce code format

* Add assertions and update comments

* fixed LocalStoreForwarder

* [skip ci] enforce code format

* [skip ci] minor: update comment

* [skip ci] minor: update comment

* Update simplify.cpp

Co-authored-by: Taichi Gardener <taichigardener@gmail.com>
Co-authored-by: Yuanming Hu <yuanming-hu@users.noreply.github.com>
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