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

Conversation

xumingkuan
Copy link
Contributor

@xumingkuan xumingkuan commented Apr 2, 2021

Related issue = #2193

This PR adds IRBuilder::LoopGuard and IRBuilder::IfGuard to make writing control flows with IR Builder easier.

These classes' objects automatically set the insertion point in O(1) time if the insertion point is never manually set in their lifetime by other functions.

[Click here for the format server]


@xumingkuan xumingkuan requested review from k-ye and BillXu2000 April 2, 2021 05:45
@xumingkuan xumingkuan marked this pull request as draft April 2, 2021 07:33
@xumingkuan xumingkuan marked this pull request as ready for review April 2, 2021 07:58
taichi/ir/ir_builder.h Outdated Show resolved Hide resolved
tests/cpp_new/ir/ir_builder_test.cpp Outdated Show resolved Hide resolved
taichi/ir/ir_builder.h Outdated Show resolved Hide resolved
Comment on lines 48 to 51
if (location_ >= 0 && location_ < loop_->parent->size() &&
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.

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.

LGTM now, thanks!

Comment on lines 48 to 51
if (location_ >= 0 && location_ < loop_->parent->size() &&
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.

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

Copy link
Member

@k-ye k-ye left a comment

Choose a reason for hiding this comment

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

Thanks! I left a few nits

taichi/ir/ir_builder.h Outdated Show resolved Hide resolved
taichi/ir/ir_builder.h Show resolved Hide resolved
taichi/ir/ir_builder.cpp Outdated Show resolved Hide resolved
Comment on lines 48 to 51
if (location_ >= 0 && location_ < loop_->parent->size() &&
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.

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.

tests/cpp_new/ir/ir_builder_test.cpp Show resolved Hide resolved
@xumingkuan xumingkuan merged commit b1bee9f into taichi-dev:master Apr 3, 2021
@xumingkuan xumingkuan deleted the builder4 branch April 7, 2021 07:21
@xumingkuan xumingkuan mentioned this pull request Apr 13, 2021
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.

3 participants