[refactor] [ir] IR system refactorings #1058

TH3CHARLie · 2020-05-25T17:13:16Z

Related issue = #988

TH3CHARLie · 2020-05-25T17:17:37Z

This PR literally follows guidelines from #988, with minimal modifications to backend code(namely adding get() to match argument types).

However, it has not yet implemented the last point on removing CompileConfig.

Try to remove dependency on get_current_program in irpasses
Use X.get_kernel().program instead

Regarding this, there are plenty of get_current_program I left untouched, they are backends and ir/frontend, FrontendForStmt, SNode.

The remaining get_current_programs in irpass are in ConstantFold::get_jit_evaluator_kernel, where no IRNode is provided

taichi/analysis/clone.cpp

taichi/transforms/type_check.cpp

xumingkuan

LGTM!

taichi/transforms/type_check.cpp

TH3CHARLie · 2020-05-26T05:37:10Z

taichi/transforms/constant_fold.cpp

@@ -113,13 +113,14 @@ class ConstantFold : public BasicStmtVisitor {
                      rhs.dt,
                      true};
    auto *ker = get_jit_evaluator_kernel(id);
-    auto &ctx = get_current_program().get_context();
+    auto &current_program = stmt->get_kernel()->program;
+    auto &ctx = current_program.get_context();


This segment has some issues with it. Please see comment below

TH3CHARLie · 2020-05-26T05:47:00Z

The current build has failed due to segfault in C++ tests (all broken tests are due to the same reason, you can simply take alg_simp/simplify_add_zero as an example).

The source of segfault lies in the above segment in constant_fold.cpp, because stmt->get_kernel() would be nullptr and therefore accessing program causes segfault.

However, I find this is rare for taichi program written in python, because in such cases, IRNode's kernel would be set. However, in C++ tests like alg_simp, I find that the IR block is constructed manually, and I suspect that some process is missing or different from construction via python AST, therefore leading to IRNode's kernel unset(nullptr).

To pass the C++ tests, one way is to fallback the above code segment to using get_current_program again. But personally I think it contradicts the idea of this PR. I'd like to know your thoughts on it @yuanming-hu @xumingkuan

updates:
I comment out the new approach to pass CI for now.

updates 2:
The similar situation happens when we change alg_simp(IRNode*, const CompileConfig &) to alg_simp(IRNode*). For taichi programs this would be fine since we can get config through IRNode::kernel->program.config. But for C++ tests, currently CompileConfigs are constructed ad-hoc and have no connection to kernels.

xumingkuan · 2020-05-26T06:13:51Z

For C++ tests, Program was constructed in this PR: #839 (comment)

Maybe we also need a Kernel for C++ tests now?

TH3CHARLie · 2020-05-26T06:22:39Z

For C++ tests, Program was constructed in this PR: #839 (comment)

Maybe we also need a Kernel for C++ tests now?

Yes, I find that program is constructed with kernel set properly. But the kernel itself is not connected to IRNode.

TH3CHARLie · 2020-05-26T07:08:13Z

Maybe we also need a Kernel for C++ tests now?

I am thinking modifying tests hugely are adding too much in this PR. Maybe we can do a hack by adding block->kernel = get_current_program().get_current_kernel only in test case. And in a later, separate PR, we make the test more reasonable

yuanming-hu · 2020-05-26T12:30:07Z

To pass the C++ tests, one way is to fallback the above code segment to using get_current_program again. But personally I think it contradicts the idea of this PR. I'd like to know your thoughts on it @yuanming-hu @xumingkuan

Let's use get_current_program for now to make the tests pass. I don't think usages of this global function can be removed in just one PR anyway :-)

TH3CHARLie · 2020-05-26T12:32:46Z

Let's use get_current_program for now to make the tests pass. I don't think usages of this global function can be removed in just one PR anyway :-)

Then it would involve changing plenty of places in C++ test code, but I assume most of them would be insignificant, especially no one would affect the check semantics itself. If that's OK for you, then I'll have a try right away

yuanming-hu · 2020-05-26T12:39:08Z

Let's use get_current_program for now to make the tests pass. I don't think usages of this global function can be removed in just one PR anyway :-)

Then it would involve changing plenty of places in C++ test code, but I assume most of them would be insignificant, especially no one would affect the check semantics itself. If that's OK for you, then I'll have a try right away

I see. Feel free to pick the most convenient solution then.

Maybe we can do a hack by adding block->kernel = get_current_program().get_current_kernel only in test case.

This sounds like the easiest way given the current situation.

xumingkuan · 2020-05-26T13:25:22Z

Yeah, since C++ tests are manually constructing Blocks, it sounds reasonable to manually set a member variable of the Block.

TH3CHARLie · 2020-05-26T14:35:52Z

Summary:

Implement all refactor TODOs listed in [refactor] [ir] IR system refactorings #988: for the last one, alg_simp and full_simplify are modified. The remaining one is compile_to_offloads, changing it would change some backend code. My personal opinion is to keep things in the IR level in this PR and address this later.
Change(or hack) C++ tests with fake kernels. After this is merged, I'd love to open an issue for better C++ tests, then we will remove these code for sure.

xumingkuan

Thanks for the hard work! Just a nit.

taichi/transforms/alg_simp.cpp

Co-authored-by: xumingkuan <xumingkuan0721@126.com>

yuanming-hu

LGTM! Thanks! @xumingkuan Please merge when everything's good on your part :-)

TH3CHARLie · 2020-05-26T18:12:42Z

Thanks for the review!

[refactor] [ir] IR system refactorings part 1

27850ff

format

7185495

yuanming-hu requested a review from xumingkuan May 25, 2020 17:22

TH3CHARLie commented May 25, 2020

View reviewed changes

taichi/analysis/clone.cpp Outdated Show resolved Hide resolved

TH3CHARLie commented May 25, 2020

View reviewed changes

taichi/transforms/type_check.cpp Show resolved Hide resolved

fix segfault

e688aa4

xumingkuan reviewed May 25, 2020

View reviewed changes

taichi/transforms/type_check.cpp Outdated Show resolved Hide resolved

TH3CHARLie added 5 commits May 26, 2020 12:25

refactor typecheck signature and misc

34a8503

fix

c621207

fix build

bddafd8

use utility function as

00a5251

fix init

c72a470

TH3CHARLie commented May 26, 2020

View reviewed changes

TH3CHARLie added 2 commits May 26, 2020 13:52

fix segfault to pass CI

113fc63

format again

3278a67

retrigger CI

5516133

TH3CHARLie added 4 commits May 26, 2020 21:38

modify test with fake kernel

0f6c631

format

ea86628

remove CompileConfig from some transform passes

45282e1

Merge branch 'master' into ir-refactor

d75f291

TH3CHARLie requested review from yuanming-hu and xumingkuan May 26, 2020 14:36

xumingkuan reviewed May 26, 2020

View reviewed changes

taichi/transforms/alg_simp.cpp Outdated Show resolved Hide resolved

Update taichi/transforms/alg_simp.cpp

b4f0e4a

Co-authored-by: xumingkuan <xumingkuan0721@126.com>

yuanming-hu approved these changes May 26, 2020

View reviewed changes

xumingkuan merged commit 2bf0bee into taichi-dev:master May 26, 2020

TH3CHARLie deleted the ir-refactor branch May 26, 2020 18:13

TH3CHARLie mentioned this pull request May 27, 2020

[Test] [refactor] Further refine C++ tests #1067

Closed

yuanming-hu mentioned this pull request May 31, 2020

[release] v0.6.7 #1094

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[refactor] [ir] IR system refactorings #1058

[refactor] [ir] IR system refactorings #1058

TH3CHARLie commented May 25, 2020

TH3CHARLie commented May 25, 2020 •

edited

Loading

xumingkuan left a comment

TH3CHARLie May 26, 2020 •

edited

Loading

TH3CHARLie commented May 26, 2020 •

edited

Loading

xumingkuan commented May 26, 2020 •

edited

Loading

TH3CHARLie commented May 26, 2020

TH3CHARLie commented May 26, 2020 •

edited

Loading

yuanming-hu commented May 26, 2020

TH3CHARLie commented May 26, 2020 •

edited

Loading

yuanming-hu commented May 26, 2020

xumingkuan commented May 26, 2020

TH3CHARLie commented May 26, 2020

xumingkuan left a comment

yuanming-hu left a comment

TH3CHARLie commented May 26, 2020

[refactor] [ir] IR system refactorings #1058

[refactor] [ir] IR system refactorings #1058

Conversation

TH3CHARLie commented May 25, 2020

TH3CHARLie commented May 25, 2020 • edited Loading

xumingkuan left a comment

Choose a reason for hiding this comment

TH3CHARLie May 26, 2020 • edited Loading

Choose a reason for hiding this comment

TH3CHARLie commented May 26, 2020 • edited Loading

xumingkuan commented May 26, 2020 • edited Loading

TH3CHARLie commented May 26, 2020

TH3CHARLie commented May 26, 2020 • edited Loading

yuanming-hu commented May 26, 2020

TH3CHARLie commented May 26, 2020 • edited Loading

yuanming-hu commented May 26, 2020

xumingkuan commented May 26, 2020

TH3CHARLie commented May 26, 2020

xumingkuan left a comment

Choose a reason for hiding this comment

yuanming-hu left a comment

Choose a reason for hiding this comment

TH3CHARLie commented May 26, 2020

TH3CHARLie commented May 25, 2020 •

edited

Loading

TH3CHARLie May 26, 2020 •

edited

Loading

TH3CHARLie commented May 26, 2020 •

edited

Loading

xumingkuan commented May 26, 2020 •

edited

Loading

TH3CHARLie commented May 26, 2020 •

edited

Loading

TH3CHARLie commented May 26, 2020 •

edited

Loading