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

Lower linearized into a series of adds and muls. #509

Merged
merged 4 commits into from
Feb 22, 2020

Conversation

xumingkuan
Copy link
Contributor

Related issue id = #464

I'm confused with those Stmt*'s and std::unique_ptr's. The code cannot pass the test now and I'm not sure if I'm writing bugs.

@yuanming-hu
Copy link
Member

One tip: unique_ptr means ownership yet raw pointers do not. Google C++ smart pointer ownership to learn more about it.

@yuanming-hu yuanming-hu changed the title Don't merge: try to lower linearized into a series of adds and muls. Lower linearized into a series of adds and muls. Feb 20, 2020
@xumingkuan xumingkuan marked this pull request as ready for review February 21, 2020 00:13
@xumingkuan
Copy link
Contributor Author

Just to confirm that the product of stmt->strides[i] will not exceed INT_MAX, right?

@yuanming-hu
Copy link
Member

Just to confirm that the product of stmt->strides[i] will not exceed INT_MAX, right?

Good question! Yes, you can assume it fits in int32.

taichi/transforms/simplify.cpp Show resolved Hide resolved
@@ -691,7 +688,32 @@ class BasicBlockSimplify : public IRVisitor {
}
Copy link
Member

Choose a reason for hiding this comment

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

The CSE here

    for (int i = 0; i < current_stmt_id; i++) {
      auto &bstmt = block->statements[i];
      if (stmt->ret_type == bstmt->ret_type) {
        auto &bstmt_data = *bstmt;
        if (typeid(bstmt_data) == typeid(*stmt)) {
          auto bstmt_ = bstmt->as<LinearizeStmt>();
          if (identical_vectors(bstmt_->inputs, stmt->inputs) &&
              identical_vectors(bstmt_->strides, stmt->strides)) {
            stmt->replace_with(bstmt.get());
            stmt->parent->erase(current_stmt_id);
            throw IRModified();
          }
        }
      }
    }

can be removed now.

stmt->insert_before_me(std::move(sum));
}
stmt->replace_with(sumptr);
stmt->parent->erase(stmt);
Copy link
Member

Choose a reason for hiding this comment

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

You can do irpass::typecheck(stmt->parent) if necessary.


// Lower into a series of adds and muls.
// Need typecheck afterwards.
if (stmt->inputs.empty()) {
Copy link
Member

Choose a reason for hiding this comment

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

Actually, no need to specialize when you have empty inputs. Empty inputs should simply be treated as inputs with zero lengths. Then you can merge two clauses of this if stmt. The extra 0 + i can be optimized by your alg_simp pass.

@@ -121,4 +121,39 @@ TI_TEST("simplify_multiply_zero_fast_math") {
TI_CHECK((*block)[0]->is<GlobalTemporaryStmt>());
}

TI_TEST("simplify_linearized_with_trivial_inputs") {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe create a test_simplify.cpp file for this test?

@yuanming-hu
Copy link
Member

Looks good in general! Just some small changes are needed.

@xumingkuan
Copy link
Contributor Author

Changes finished :)

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.

Awesome!!!

@yuanming-hu yuanming-hu merged commit 73cb534 into taichi-dev:master Feb 22, 2020
@xumingkuan xumingkuan deleted the linearized branch March 6, 2020 22:18
@xumingkuan xumingkuan mentioned this pull request Mar 25, 2020
18 tasks
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

2 participants