Make atomic_add() return value (should be ready to review...)

[k-ye]

Issue #332

I've finally got things working (only tested on CPU, sadly...). I had to make a few changes to IR passes to adapt to this change. Specifically:

1. Demote atomics should replace the demoted stmt with the old value loaded before add, as you've pointed out in atomic_add return value #332 (comment)
2. LocalStoreStmt cannot be simplified in a few cases where atomic ops appeared
3. IfStmt cannot move certain LocalStore out of the branch clauses if the store depends on atomic operations. This one is somewhat nontrivial in this change, as it does two things
   1. it no longer moves LocalStore that depend on atomics, for correctness reasons
   2. it no longer moves AllocaStmt that is 1) in the clause, and 2) the dest of LocalStore. This is for performance reason, as otherwise some dead code cannot be eliminated.

I wonder if this PR is getting too big, especially the IfStmt -> AllocaStmt part, as that is logically a separate thing, and lacks proper unit tests... If you think so, I can separate this into two PRs.

If you think the code looks fine, can you help test the GPU version? Or is Travis CI able to test that?

[k-ye]

Hmm, the tests passed, but the CI failed due to missing PYPI_PWD...

Speaking of unit tests, I feel like they are great to have, especially for us new contributors, as they are effective and a good documentation of the code itself. (I initially found the IfStmt because renderer.py was broken, and it was too much fun debugging the IR.. lol) The downside is that it slows the development a bit (but it can also save time debugging..!) If you like the idea, I can try adding some basic unit tests :)

[k-ye]

hmm, giving it more thoughts and i realized that my changes of the IfStmt is completely off, because i only kept the LocalStore immediately depending on that AtomicOp, but then those stmts depending on this LocalStore must also be kept, and so on so forth.

The simplest correct fix is probably to set plain_clause to false if AtomicOp ever appears in the clause. But that disables many optimizations since += is implemented via atomics... :-/

---

More thoughts: allow the optimization only if the return value of AtomicOp is unused... This should be straightforward, because for those LocalStores generated to hold AtomicOp's value, if the stored data is not used in any way, it will get simplified (no following load). Then, we can check if AtomicOp is used simply by checking if it is an operand of any subsequent stmt in the if clause. It also doesn't introduce additional cost when users use +=.

[yuanming-hu]

Sorry I've been traveling these days and have very limited time to code. I will settle down and get back to you on Thursday. I think what you have done is awesome but I need a little bit of time to take a careful pass.

Quick answers for now:

1. There's no free CI online with GPUs, but I have access to two GPU machines at MIT that I use as buildbots. I'll test your implementation once I'm back.
2. I agree with your opinion on unit tests. I would say the time unit testing saved on debugging overweight the time you spend writing the tests. Writing a compiler without unit testing will basically stop the development progress, since the developers will have no confidence to modify anything :-)

Thank you so much for contributing!!

[k-ye]

No worries at all!! (太客气了...!) Please take your time (i'm doing it mostly for fun)!

[yuanming-hu]

Thanks so much for contributing! I have taken a close look at the implementation and everything makes sense to me. The only part that is not yet trivially correct to me is simplify.cpp, which I will take a closer look after merging this PR, together with #387.

[k-ye]

Thank you for the review! FYI I'd be totally cool if some part didn't seem intuitive, and you wanted some more refactoring or testings before the merge...

With this feature, I was finally able to port my position based fluid demo to Taichi, which took about two nights 😆 Again, thanks for the amazing work! (I'd be more than proud to add this to examples/ :D )

[yuanming-hu]

Wow this looks really nice! Please feel free to add it to examples and make a PR. After the memory allocator is redesigned, we can also consider adding spatial hashing to make it more efficient and probably scale it up to 3D!

(I plan to finish the performance tuning of the LLVM backend by the end of Jan and then redo the memory allocator.)

[yuanming-hu]

Btw, this PR passed all tests on GPUs. Thanks again :-)

[k-ye]

Nice! Spatial hashing was definitely one of the things i was looking for, but dense is also good enough for now :)