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

Jump to bottom

[refactor] Remove the global begin_stmt and end_stmt #1034

Merged
merged 2 commits into from
May 23, 2020
Merged

[refactor] Remove the global begin_stmt and end_stmt #1034

merged 2 commits into from
May 23, 2020

Conversation

k-ye
Copy link
Member

@k-ye k-ye commented May 21, 2020 •
edited
Loading

"One less global variable is a bit less evil in the world." -- Nobody 233 B.C.

As part of the refactor, I also:

  • Reordered a few static function args so that they match the corresponding Constructor's order
  • Added const & to a few stmt_to_offloaded maps.

Related issue = N/A
WANT_LGTM = any

[Click here for the format server]

@k-ye k-ye requested review from xumingkuan and archibate May 21, 2020 13:56
archibate
archibate reviewed May 21, 2020
View reviewed changes
Copy link
Collaborator

@archibate archibate left a comment

Choose a reason for hiding this comment

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

LGTM + nits :)

taichi/transforms/offload.cpp
Comment on lines -334 to +343
std::unordered_map<Stmt *, Stmt *> stmt_to_offloaded)
const std::unordered_map<Stmt *, Stmt *> &stmt_to_offloaded,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any reason why const & here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. If you look at the entire call stack, it was like:

offload()
\- FixCrossOffloadReferences::run()
    \- FixCrossOffloadReferences::FixCrossOffloadReferences()

Before this PR, only the last call used move, while the first two didn't (likely because we forgot to), so we still made two unnecessary copies. While move is a modern feature, IMO the easiest way to avoid copy remains using const &. (move also means transferring ownership, which is really useful when data cannot be copy constructed, e.g. std::unique_ptr). Does this make sense?

taichi/transforms/offload.cpp Outdated
@@ -14,20 +14,18 @@ namespace {
// Offloaded local variables to its offset in the global tmps memory.
using StmtToOffsetMap = std::unordered_map<const Stmt *, std::size_t>;

std::unique_ptr<std::unordered_map<OffloadedStmt *, Stmt *>> begin_stmt,
end_stmt;
struct OffloadedToBeginEndStmts {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Name nit: why to?

Suggested change
struct OffloadedToBeginEndStmts {
struct OffloadedRanges {

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice! Since many places need to be changed, I will make the change locally.

taichi/transforms/offload.cpp Outdated
@@ -77,9 +77,11 @@ class Offloader {
}
}
assemble_serial_statements();
return offloaded_to_begin_end;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
return offloaded_to_begin_end;
return offloaded_ranges;

Copy link
Member Author

Choose a reason for hiding this comment

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

Ditto

@k-ye
Copy link
Member Author

k-ye commented May 21, 2020

@archibate If you do approve, could you hit the Approve button?

@k-ye k-ye requested a review from archibate May 21, 2020 15:20
archibate
archibate approved these changes May 21, 2020
View reviewed changes
Copy link
Collaborator

@archibate archibate left a comment

Choose a reason for hiding this comment

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

LGTMig, hitting Approve button :)

xumingkuan
xumingkuan reviewed May 21, 2020
View reviewed changes
Copy link
Contributor

@xumingkuan xumingkuan left a comment

Choose a reason for hiding this comment

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

LGTM in general but I wonder why OffloadedRanges *const offloaded_ranges_; is used rather than const OffloadedRanges &offloaded_ranges_; -- I think it's better to use a const reference if we don't modify it.

@k-ye
Copy link
Member Author

k-ye commented May 22, 2020 •
edited
Loading

why OffloadedRanges *const offloaded_ranges_; is used rather than const OffloadedRanges &offloaded_ranges_; -- I think it's better to use a const reference if we don't modify it.

It's because we do need the non-const value of the hash maps in const OffloadedRanges&, therefore we cannot mark it const (unfortunately). See https://github.com/taichi-dev/taichi/pull/1034/files#diff-3960fe96f8a142ecb79201fd25fa3335R212-R213 and

taichi/taichi/transforms/offload.cpp

Line 223 in 28ae40c

void test_and_allocate(Stmt *stmt) {
.

BTW it seems that test_and_allocate()'s arg can be made const, but maybe we should clean that up later.

@xumingkuan
Copy link
Contributor

why OffloadedRanges *const offloaded_ranges_; is used rather than const OffloadedRanges &offloaded_ranges_; -- I think it's better to use a const reference if we don't modify it.

It's because we do need the non-const value of the hash maps in const OffloadedRanges&, therefore we cannot mark it const (unfortunately). See https://github.com/taichi-dev/taichi/pull/1034/files#diff-3960fe96f8a142ecb79201fd25fa3335R212-R213 and

taichi/taichi/transforms/offload.cpp

Line 223 in 28ae40c

void test_and_allocate(Stmt *stmt) {

.
BTW it seems that test_and_allocate()'s arg can be made const, but maybe we should clean that up later.

I see. So if const OffloadedRanges& is used, begin->second will have type const Stmt *?

BTW is there any advantage of OffloadedRanges *const over OffloadedRanges *? Does it only imply that the pointer itself can't be modified?

@k-ye
Copy link
Member Author

k-ye commented May 22, 2020

I see. So if const OffloadedRanges& is used, begin->second will have type const Stmt *?

Yes, see find. There are two overloads on it, based on the constness of unordered_map itself. If the map is const, then only (2) is available, which implies constness of its kv as well.

Does it only imply that the pointer itself can't be modified?

Yes, since T* const is basically a T&, this is also used to convey the fact that the object doesn't own the pointer.

@k-ye k-ye requested a review from xumingkuan May 22, 2020 02:38
@k-ye k-ye closed this May 23, 2020
@k-ye k-ye reopened this May 23, 2020
@k-ye k-ye merged commit 8b1db49 into taichi-dev:master May 23, 2020
@k-ye k-ye deleted the offl branch May 23, 2020 01:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xumingkuan xumingkuan xumingkuan approved these changes

@archibate archibate archibate approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@k-ye @xumingkuan @archibate