Fix index truncation in argmin/max for large tensors#33310
Closed
peterbell10 wants to merge 3 commits intopytorch:masterfrom
Closed
Fix index truncation in argmin/max for large tensors#33310peterbell10 wants to merge 3 commits intopytorch:masterfrom
peterbell10 wants to merge 3 commits intopytorch:masterfrom
Conversation
09bb2b3 to
f2b4178
Compare
💊 CircleCI build failures summary and remediationsAs of commit c26c2f6:
Detailed failure analysisOne may explore the probable reasons each build failed interactively on the Dr. CI website. 🕵️ 1 new failure recognized by patternsThe following build failures do not appear to be due to upstream breakage:
|
Contributor
|
nifty. @VitalyFedyunin do you think you can take full reviewership on this or should I look? |
Contributor
|
Or maybe Natalia is the best reviewer here. |
Contributor
facebook-github-bot
left a comment
There was a problem hiding this comment.
@VitalyFedyunin has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Collaborator
Author
|
Test failure looks genuine, but for some reason it's the CPU version that failed: |
ngimel
approved these changes
Feb 14, 2020
Collaborator
ngimel
left a comment
There was a problem hiding this comment.
Approving, modulo failing test. Do you need help with debugging it?
Collaborator
Author
|
It looks like there are places where the cpu reduction also splits iterators. I'll see if the same fix will help for there. |
4fe198f to
c26c2f6
Compare
Collaborator
Author
|
I applied the fix to the CPU reduction but it turns out the actual cause here was just that I was assigning |
Contributor
facebook-github-bot
left a comment
There was a problem hiding this comment.
@ngimel has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Contributor
|
I hope #33370 doesn't intervene with this one |
Collaborator
|
#33370 is unrelated. |
Contributor
Contributor
|
Are there any consequences about this mixed 64bit/32bit indexing? e.g. is it faster to do argmax across a contiguous small chunk vs a discontiguous small chunk that may require 64bit indexing? |
Collaborator
Author
This PR only introduces 1 extra add per reduced element in each kernel, so I don't expect performance to change much.
There is a potential cost from splitting the operation over multiple kernel launches; it will limit the parallelism available within each kernel. For large contiguous chunks, that's inconsequential but, in the extreme, I suppose a slice However, I'd note that this has always been the case for |
Contributor
|
So in theory there is tradeoff between multiple kernel launches that do 32-bit indexing and a single kernel launch that must do 64-bit indexing? |
Collaborator
Author
|
In theory, yes. In practice I don't think it matters much. 32-bit indexing covers ~2GB of memory so you could only have 10-20 tensor splits before you've covered all the memory in a top of the line Quadro card. |
facebook-github-bot
pushed a commit
that referenced
this pull request
Feb 20, 2020
Summary: Fixes #32863, (together with #33310 for the `TensorIterator` reductions) This adds 64-bit indexed kernels for `THC_reduceDimIndex` and uses `THCTensor_canUse32BitIndexMath` to switch between the two at runtime. I have a test for this locally but haven't included it here because `max` is much slower than `argmax`. To the point where the test takes several minutes to call max on just one `2**32` element tensor. That seems excessive, even for a slow test but I can push it if preferred. Pull Request resolved: #33405 Differential Revision: D20010769 Pulled By: ezyang fbshipit-source-id: a8a86f662598d5fade4d90448436418422c699a3
ttumiel
pushed a commit
to ttumiel/pytorch
that referenced
this pull request
Mar 4, 2020
Summary: Fixes the `TensorIterator` parts of pytorch#32863 (THC is still broken) `TensorIterator::split` now keeps track of the `view_offsets` into the full tensor range. With this, I can take the base offset for the reduced dimension and translate partial results from the sub-iter into the index range of the full tensor. This happens only once for each intermediate result, so we should still benefit from the performance of 32-bit indexing in loops. Pull Request resolved: pytorch#33310 Differential Revision: D19906136 Pulled By: ngimel fbshipit-source-id: 3372ee4b8d5b115a53be79aeafc52e80ff9c490b
ttumiel
pushed a commit
to ttumiel/pytorch
that referenced
this pull request
Mar 4, 2020
Summary: Fixes pytorch#32863, (together with pytorch#33310 for the `TensorIterator` reductions) This adds 64-bit indexed kernels for `THC_reduceDimIndex` and uses `THCTensor_canUse32BitIndexMath` to switch between the two at runtime. I have a test for this locally but haven't included it here because `max` is much slower than `argmax`. To the point where the test takes several minutes to call max on just one `2**32` element tensor. That seems excessive, even for a slow test but I can push it if preferred. Pull Request resolved: pytorch#33405 Differential Revision: D20010769 Pulled By: ezyang fbshipit-source-id: a8a86f662598d5fade4d90448436418422c699a3
facebook-github-bot
pushed a commit
that referenced
this pull request
Jun 16, 2020
Summary: Closes gh-39060 The `TensorIterator` splitting is based on `can_use_32bit_indexing` which assumes 32-bit signed ints, so we can get away with just 2**31 as the axis length. Also tested on an old commit that I can reproduce the test failure on just a 1d tensor, overall quartering the memory requirement for the test. https://github.com/pytorch/pytorch/blob/4c7d81f8479bce320cc11d1eb3adaf8ab0b90099/aten/src/ATen/native/TensorIterator.cpp#L879 For reference, the test was first added in gh-33310. Pull Request resolved: #40036 Differential Revision: D22068690 Pulled By: ezyang fbshipit-source-id: 83199fd31647d1ef106b08f471c0e9517d3516e3
xwang233
pushed a commit
to xwang233/pytorch
that referenced
this pull request
Jun 20, 2020
Summary: Closes pytorchgh-39060 The `TensorIterator` splitting is based on `can_use_32bit_indexing` which assumes 32-bit signed ints, so we can get away with just 2**31 as the axis length. Also tested on an old commit that I can reproduce the test failure on just a 1d tensor, overall quartering the memory requirement for the test. https://github.com/pytorch/pytorch/blob/4c7d81f8479bce320cc11d1eb3adaf8ab0b90099/aten/src/ATen/native/TensorIterator.cpp#L879 For reference, the test was first added in pytorchgh-33310. Pull Request resolved: pytorch#40036 Differential Revision: D22068690 Pulled By: ezyang fbshipit-source-id: 83199fd31647d1ef106b08f471c0e9517d3516e3
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the
TensorIteratorparts of #32863 (THC is still broken)TensorIterator::splitnow keeps track of theview_offsetsinto the full tensor range. With this, I can take the base offset for the reduced dimension and translate partial results from the sub-iter into the index range of the full tensor. This happens only once for each intermediate result, so we should still benefit from the performance of 32-bit indexing in loops.