Skip to content

[NNC] Fix a bug in SplitWithMask when splitting multiple times #45141

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
Closed
nickgg wants to merge 1 commit into from
Closed

[NNC] Fix a bug in SplitWithMask when splitting multiple times #45141

nickgg wants to merge 1 commit into from

Conversation

@nickgg
Copy link
Contributor

@nickgg nickgg commented Sep 22, 2020

When doing a splitWithMask we only mask if the loop extent is not cleanly divide by the split factor. However, the logic does not simplify so any nontrivial loop extents will always cause a mask to be added, e.g. if the loop had been previously split. Unlike splitWithTail, the masks added by splitWithMask are always overhead and we don't have the analysis to optimize them out if they are unnecessary, so it's good to avoid inserting them if we can.

The fix is just to simplify the loop extents before doing the extent calculation.

@nickgg nickgg requested a review from apaszke as a code owner September 22, 2020 17:32
@facebook-github-bot facebook-github-bot added the oncall: jit Add this issue/PR to JIT oncall triage queue label Sep 22, 2020
navahgar
navahgar approved these changes Sep 22, 2020
View reviewed changes
Copy link
Contributor

@navahgar navahgar left a comment

Choose a reason for hiding this comment

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

LGTM

@dr-ci
Copy link

dr-ci bot commented Sep 22, 2020 •
edited
Loading

💊 CI failures summary and remediations

As of commit 5d3cf18 (more details on the Dr. CI page):

  • 1/1 failures introduced in this PR

🕵️ 1 new failure recognized by patterns

The following CI failures do not appear to be due to upstream breakages:

See CircleCI build pytorch_bazel_build (1/1)

Step: "Bazel Build" (full log | diagnosis details | 🔁 rerun)

Sep 23 15:56:27 caused by: Connection refused (os error 111) 
Sep 23 15:56:27 ++++ extract_trap_cmd 
Sep 23 15:56:27 ++++ printf '%s\n' '' 
Sep 23 15:56:27 +++ printf '%s\n' cleanup 
Sep 23 15:56:27 ++ trap -- ' 
Sep 23 15:56:27 cleanup' EXIT 
Sep 23 15:56:27 ++ [[ pytorch-linux-xenial-py3.6-gcc7-bazel-build != *pytorch-win-* ]] 
Sep 23 15:56:27 ++ which sccache 
Sep 23 15:56:27 ++ sccache --stop-server 
Sep 23 15:56:27 Stopping sccache server... 
Sep 23 15:56:27 error: couldn't connect to server 
Sep 23 15:56:27 caused by: Connection refused (os error 111) 
Sep 23 15:56:27 ++ true 
Sep 23 15:56:27 ++ rm /var/lib/jenkins/sccache_error.log 
Sep 23 15:56:27 rm: cannot remove '/var/lib/jenkins/sccache_error.log': No such file or directory 
Sep 23 15:56:27 ++ true 
Sep 23 15:56:27 ++ [[ pytorch-linux-xenial-py3.6-gcc7-bazel-build == *rocm* ]] 
Sep 23 15:56:27 ++ SCCACHE_ERROR_LOG=/var/lib/jenkins/sccache_error.log 
Sep 23 15:56:27 ++ SCCACHE_IDLE_TIMEOUT=1200 
Sep 23 15:56:27 ++ RUST_LOG=sccache::server=error 
Sep 23 15:56:27 ++ sccache --start-server 
Sep 23 15:56:27 Starting sccache server...
This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions on the GitHub issue tracker or post in the (internal) Dr. CI Users group.

See how this bot performed.

This comment has been revised 5 times.

facebook-github-bot
facebook-github-bot reviewed Sep 23, 2020
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@nickgg has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Sep 23, 2020

@nickgg merged this pull request in 9e206ee.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@facebook-github-bot facebook-github-bot facebook-github-bot left review comments

@bertmaher bertmaher Awaiting requested review from bertmaher

@ZolotukhinM ZolotukhinM Awaiting requested review from ZolotukhinM

@apaszke apaszke Awaiting requested review from apaszke

+1 more reviewer

@navahgar navahgar navahgar approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Merged oncall: jit Add this issue/PR to JIT oncall triage queue

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@nickgg @facebook-github-bot @navahgar @mruberry