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

[ir] Add RAII guards to IR Builder #2242

Merged
merged 8 commits into from
Apr 3, 2021
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 32 additions & 0 deletions taichi/ir/ir_builder.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,38 @@ void IRBuilder::set_insertion_point_to_false_branch(IfStmt *if_stmt) {
set_insertion_point({if_stmt->false_statements.get(), 0});
}

IRBuilder::LoopGuard::~LoopGuard() {
if (location_ >= 0 && location_ < loop_->parent->size() &&
xumingkuan marked this conversation as resolved.
Show resolved Hide resolved
loop_->parent->statements[location_].get() == loop_) {
// faster than set_insertion_point_to_after()
builder_.set_insertion_point({loop_->parent, location_ + 1});
Copy link
Member

Choose a reason for hiding this comment

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

At a high level, what does this condition mean? Something like "the number of statements before the loop statements stays the same"? I'm wondering in what scenario the number of statements changes - it sounds to me that when using the guard the users are only supposed to modify the statements within the guarded scope.

Copy link
Contributor Author

@xumingkuan xumingkuan Apr 2, 2021

Choose a reason for hiding this comment

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

Something like "the number of statements before the loop statements stays the same"?

Yes, exactly. This condition is to prevent some random things like

auto *loop = builder.create_range_for(zero, ten);
{
  IRBuilder::LoopGuard _(builder, loop);
  builder.create_add(...);
  builder.set_insertion_point_to(some_random_insertion_point);
  builder.create_add(...);  // if some_random_insertion_point is in the same block but before the loop, it may cause trouble...
}  // here I set the insertion point to a *defined* place -- the point after the loop.

or

auto *loop = builder.create_range_for(zero, ten);
builder.create_add(...);  // this is inserted to the place after the loop so we can't locate the loop in O(1) then
{
  IRBuilder::LoopGuard _(builder, loop);
  builder.create_add(...);
}

Copy link
Member

Choose a reason for hiding this comment

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

OK sounds good! Let's add these comments to the code and this PR is ready to merge!

Copy link
Member

Choose a reason for hiding this comment

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

I set the insertion point to a defined place -- the point after the loop.

Could you add this comment to the header ? It describes the API's behavior.

} else {
builder_.set_insertion_point_to_after(loop_);
}
}

IRBuilder::IfGuard::IfGuard(IRBuilder &builder,
IfStmt *if_stmt,
bool true_branch)
: builder_(builder), if_stmt_(if_stmt) {
location_ = (int)if_stmt_->parent->size() - 1;
if (true_branch) {
builder_.set_insertion_point_to_true_branch(if_stmt_);
} else {
builder_.set_insertion_point_to_false_branch(if_stmt_);
}
}

IRBuilder::IfGuard::~IfGuard() {
if (location_ >= 0 && location_ < if_stmt_->parent->size() &&
if_stmt_->parent->statements[location_].get() == if_stmt_) {
// faster than set_insertion_point_to_after()
builder_.set_insertion_point({if_stmt_->parent, location_ + 1});
} else {
builder_.set_insertion_point_to_after(if_stmt_);
}
}

RangeForStmt *IRBuilder::create_range_for(Stmt *begin,
Stmt *end,
int vectorize,
Expand Down
65 changes: 50 additions & 15 deletions taichi/ir/ir_builder.h
Original file line number Diff line number Diff line change
Expand Up @@ -25,28 +25,28 @@ class IRBuilder {
std::unique_ptr<IRNode> extract_ir();

// General inserter. Returns stmt.get().
template <typename XxxStmt>
XxxStmt *insert(std::unique_ptr<XxxStmt> &&stmt) {
template <typename XStmt>
XStmt *insert(std::unique_ptr<XStmt> &&stmt) {
return insert(std::move(stmt), &insert_point_);
}

// Insert to a specific insertion point.
template <typename XxxStmt>
static XxxStmt *insert(std::unique_ptr<XxxStmt> &&stmt,
InsertPoint *insert_point) {
template <typename XStmt>
static XStmt *insert(std::unique_ptr<XStmt> &&stmt,
InsertPoint *insert_point) {
return insert_point->block
->insert(std::move(stmt), insert_point->position++)
->template as<XxxStmt>();
->template as<XStmt>();
}

void set_insertion_point(InsertPoint new_insert_point);
void set_insertion_point_to_after(Stmt *stmt);
void set_insertion_point_to_before(Stmt *stmt);
void set_insertion_point_to_true_branch(IfStmt *if_stmt);
void set_insertion_point_to_false_branch(IfStmt *if_stmt);
template <typename XxxStmt>
void set_insertion_point_to_loop_begin(XxxStmt *loop) {
using DecayedType = typename std::decay_t<XxxStmt>;
template <typename XStmt>
void set_insertion_point_to_loop_begin(XStmt *loop) {
using DecayedType = typename std::decay_t<XStmt>;
if constexpr (!std::is_base_of_v<Stmt, DecayedType>) {
TI_ERROR("The argument is not a statement.");
}
Expand All @@ -59,6 +59,41 @@ class IRBuilder {
}
}

// RAII handles insertion points automatically.
class LoopGuard {
private:
xumingkuan marked this conversation as resolved.
Show resolved Hide resolved
IRBuilder &builder_;
Stmt *loop_;
int location_;

public:
template <typename XStmt>
explicit LoopGuard(IRBuilder &builder, XStmt *loop)
: builder_(builder), loop_(loop) {
location_ = (int)loop->parent->size() - 1;
builder_.set_insertion_point_to_loop_begin(loop);
}
~LoopGuard();
};
class IfGuard {
private:
xumingkuan marked this conversation as resolved.
Show resolved Hide resolved
IRBuilder &builder_;
IfStmt *if_stmt_;
int location_;

public:
explicit IfGuard(IRBuilder &builder, IfStmt *if_stmt, bool true_branch);
~IfGuard();
};

template <typename XStmt>
LoopGuard get_loop_guard(XStmt *loop) {
return LoopGuard(*this, loop);
}
IfGuard get_if_guard(IfStmt *if_stmt, bool true_branch) {
return IfGuard(*this, if_stmt, true_branch);
}

// Control flows.
RangeForStmt *create_range_for(Stmt *begin,
Stmt *end,
Expand Down Expand Up @@ -164,9 +199,9 @@ class IRBuilder {
const std::vector<Stmt *> &indices);
ExternalPtrStmt *create_external_ptr(ArgLoadStmt *ptr,
const std::vector<Stmt *> &indices);
template <typename XxxStmt>
GlobalLoadStmt *create_global_load(XxxStmt *ptr) {
using DecayedType = typename std::decay_t<XxxStmt>;
template <typename XStmt>
GlobalLoadStmt *create_global_load(XStmt *ptr) {
using DecayedType = typename std::decay_t<XStmt>;
if constexpr (!std::is_base_of_v<Stmt, DecayedType>) {
TI_ERROR("The argument is not a statement.");
}
Expand All @@ -177,9 +212,9 @@ class IRBuilder {
TI_ERROR("Statement {} is not a global pointer.", ptr->name());
}
}
template <typename XxxStmt>
void create_global_store(XxxStmt *ptr, Stmt *data) {
using DecayedType = typename std::decay_t<XxxStmt>;
template <typename XStmt>
void create_global_store(XStmt *ptr, Stmt *data) {
using DecayedType = typename std::decay_t<XStmt>;
if constexpr (!std::is_base_of_v<Stmt, DecayedType>) {
TI_ERROR("The argument is not a statement.");
}
Expand Down
10 changes: 6 additions & 4 deletions tests/cpp_new/ir/ir_builder_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -44,10 +44,12 @@ TEST(IRBuilder, RangeFor) {
auto *zero = builder.get_int32(0);
auto *ten = builder.get_int32(10);
auto *loop = builder.create_range_for(zero, ten);
builder.set_insertion_point_to_loop_begin(loop);
auto *index = builder.get_loop_index(loop, 0);
builder.set_insertion_point_to_after(loop);
auto *ret = builder.create_return(zero);
Stmt *index;
{
auto _ = builder.get_loop_guard(loop);
index = builder.get_loop_index(loop, 0);
}
[[maybe_unused]] auto *ret = builder.create_return(zero);
EXPECT_EQ(zero->parent->size(), 4);
ASSERT_TRUE(loop->is<RangeForStmt>());
auto *loopc = loop->cast<RangeForStmt>();
xumingkuan marked this conversation as resolved.
Show resolved Hide resolved
Expand Down