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.

Sign up for GitHub

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
+128 −19
Merged

[ir] Add RAII guards to IR Builder #2242

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
8d53143
[ir] Add RAII guards to IR Builder
xumingkuan Apr 2, 2021
e217cda
fix CE and add an optimization
xumingkuan Apr 2, 2021
77ac7b6
Apply suggestions from code review
xumingkuan Apr 2, 2021
187f1fc
Add get_loop_guard and rename XxxStmt
xumingkuan Apr 2, 2021
6fbc5e0
Add signed conversion
xumingkuan Apr 2, 2021
19e709d
code format
xumingkuan Apr 2, 2021
56ddfa6
Apply review
xumingkuan Apr 3, 2021
2b932ff
Apply review
xumingkuan Apr 3, 2021
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
View file
Open in desktop
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 •
edited
Loading

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_ = 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
27 changes: 27 additions & 0 deletions taichi/ir/ir_builder.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,33 @@ 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 XxxStmt>
explicit LoopGuard(IRBuilder &builder, XxxStmt *loop)
xumingkuan marked this conversation as resolved.
Show resolved Hide resolved
: builder_(builder), loop_(loop) {
location_ = loop->parent->size() - 1;
xumingkuan marked this conversation as resolved.
Show resolved Hide resolved
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();
};

// Control flows.
RangeForStmt *create_range_for(Stmt *begin,
Stmt *end,
Expand Down
10 changes: 6 additions & 4 deletions tests/cpp_new/ir/ir_builder_test.cpp
View file
Open in desktop
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;
{
IRBuilder::LoopGuard _(builder, loop);
xumingkuan marked this conversation as resolved.
Show resolved Hide resolved
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