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

[bug] Fix bug that kernel names are not correctly captured by the profiler #5651

Merged
merged 4 commits into from
Aug 8, 2022

Conversation

PGZXB
Copy link
Contributor

@PGZXB PGZXB commented Aug 5, 2022

Related issue = fixes #5643

@netlify
Copy link

netlify bot commented Aug 5, 2022

Deploy Preview for docsite-preview canceled.

Name Link
🔨 Latest commit dee298d
🔍 Latest deploy log https://app.netlify.com/sites/docsite-preview/deploys/62ed3f4b0e71a90009acc303

@PGZXB PGZXB added this to the Taichi v1.1.0 milestone Aug 5, 2022
@PGZXB PGZXB requested a review from lin-hitonami August 6, 2022 02:02
@PGZXB PGZXB marked this pull request as draft August 8, 2022 02:04
@PGZXB PGZXB removed this from the Taichi v1.1.0 milestone Aug 8, 2022
@PGZXB PGZXB marked this pull request as ready for review August 8, 2022 02:28
@PGZXB PGZXB requested review from lin-hitonami and removed request for lin-hitonami August 8, 2022 02:28
@PGZXB PGZXB added this to the Taichi v1.1.0 milestone Aug 8, 2022
Copy link
Contributor

@lin-hitonami lin-hitonami left a comment

Choose a reason for hiding this comment

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

Per offline discussion, it's hard to determine whether a task is cached because the profiler cannot access the kernel information. It needs many refactoring to let the profiler know whether the task is cached. This is a temporary solution that enables the profiler to deduce the real name of a kernel from the mangled name. Special characters cannot be used because they cause error on windows. See

std::string convert(std::string new_name) {
// Evil C++ mangling on Windows will lead to "unsupported characters in
// symbol" error in LLVM PTX printer. Convert here.
for (int i = 0; i < (int)new_name.size(); i++) {
if (new_name[i] == '@')
new_name.replace(i, 1, "_at_");
if (new_name[i] == '?')
new_name.replace(i, 1, "_qm_");
if (new_name[i] == '$')
new_name.replace(i, 1, "_dl_");
if (new_name[i] == '<')
new_name.replace(i, 1, "_lb_");
if (new_name[i] == '>')
new_name.replace(i, 1, "_rb_");
TI_ASSERT(std::isalpha(new_name[i]) || std::isdigit(new_name[i]) ||
new_name[i] == '_' || new_name[i] == '.');
}
if (!new_name.empty())
TI_ASSERT(isalpha(new_name[0]) || new_name[0] == '_' || new_name[0] == '.');
return new_name;
}
. Therefore for now we use the checksum to determine whether the name is mangled or not.

@PGZXB PGZXB merged commit f335e00 into taichi-dev:master Aug 8, 2022
@ailzhang
Copy link
Contributor

ailzhang commented Aug 8, 2022

@PGZXB could you cherrypick this commit into rc-v1.1.0 and send another PR against that branch as well? Thanks! :D

PGZXB added a commit to PGZXB/taichi that referenced this pull request Aug 8, 2022
…filer (taichi-dev#5651)

* [bug] Fix bug that kernel names are not correctly captured by the profiler

* [pre-commit.ci] auto fixes from pre-commit.com hooks

* Fix w-error

* Make LLVM AOT happy

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
ailzhang pushed a commit that referenced this pull request Aug 8, 2022
…filer (#5651) (#5669)

* [bug] Fix bug that kernel names are not correctly captured by the profiler

* [pre-commit.ci] auto fixes from pre-commit.com hooks

* Fix w-error

* Make LLVM AOT happy

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
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.

kernel names are not correctly captured by the profiler
3 participants