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

Don't replace reduction init axis with new axis if bound to a thread. #3408

Merged
merged 6 commits into from
Aug 12, 2019

Conversation

csarofeen
Copy link
Contributor

Fixes #3407

@tqchen tqchen added the status: need test case need test cases to cover the change label Jun 27, 2019
@tqchen
Copy link
Member

tqchen commented Jun 27, 2019

Thanks for the PR, sorry for the delayed review as many of folks I know in the community are traveling to FCRC recently. Can you please add a regression test case to prevent it from happening again?

@csarofeen
Copy link
Contributor Author

Sure, I should be able to base it off the repro in the issue. I'll put it on my todo list but may not get to it for a while. I've been prototyping some kernels in TE, which is why the few small bug fixes. When I get a break I'll come back and write some tests.

@tqchen
Copy link
Member

tqchen commented Jul 8, 2019

@csarofeen can you add a test case?

@tqchen tqchen added the status: need update need update based on feedbacks label Jul 8, 2019
@tqchen
Copy link
Member

tqchen commented Jul 23, 2019

ping @csarofeen

@csarofeen
Copy link
Contributor Author

Sorry this was so delayed, was actually preparing a talk on TE for CUDA code generation. Simple test added @tqchen

@csarofeen
Copy link
Contributor Author

I can't tell if the linter error is from my change or not. I tried to get it running locally but I got a docker build error.


mo, _ = s[B].split(B.op.axis[0], 32)
s[B].bind(mo, tvm.thread_axis("blockIdx.x"))
fcuda = tvm.build(s, [A, B], "cuda")
Copy link
Member

Choose a reason for hiding this comment

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

Add guard to this test (see Line 143). Because some test environments do not have a gpu.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, updated.

@csarofeen
Copy link
Contributor Author

@merrymercy @tqchen I rebased at top of tree and added 2 tests to cover: #3444 and #3382 hope that's okay.

@csarofeen
Copy link
Contributor Author

Ping. If this looks good I can rebase so it can be merged.

@tqchen
Copy link
Member

tqchen commented Aug 9, 2019

Thanks @csarofeen please rebase

@csarofeen
Copy link
Contributor Author

Looks like this got stuck, is there a way to retry the tests?

@tqchen tqchen merged commit d482512 into apache:master Aug 12, 2019
@tqchen
Copy link
Member

tqchen commented Aug 12, 2019

Thanks @csarofeen , this PR is now merged!

@tqchen tqchen added status: accepted and removed status: need test case need test cases to cover the change status: need update need update based on feedbacks labels Aug 12, 2019
wweic pushed a commit to neo-ai/tvm that referenced this pull request Aug 16, 2019
…apache#3408)

* Don't replace reduction init axis with new axis if bound to a thread.

* Linter.

* Reduce bind test case.

* Guard test on CUDA support.

* [CUDA TE TESTS] Add rfactor predicate test, add global bx and tx.

* [CUDA TE TESTS] Add loop partition test for simple rfactor case.
anijain2305 pushed a commit to anijain2305/tvm that referenced this pull request Aug 22, 2019
…apache#3408)

* Don't replace reduction init axis with new axis if bound to a thread.

* Linter.

* Reduce bind test case.

* Guard test on CUDA support.

* [CUDA TE TESTS] Add rfactor predicate test, add global bx and tx.

* [CUDA TE TESTS] Add loop partition test for simple rfactor case.
wweic pushed a commit to neo-ai/tvm that referenced this pull request Sep 6, 2019
…apache#3408)

* Don't replace reduction init axis with new axis if bound to a thread.

* Linter.

* Reduce bind test case.

* Guard test on CUDA support.

* [CUDA TE TESTS] Add rfactor predicate test, add global bx and tx.

* [CUDA TE TESTS] Add loop partition test for simple rfactor case.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Binding on internal reduction axis.
3 participants