This repository was archived by the owner on Jul 1, 2025. It is now read-only.
[OpenCL] Fix bugs in BatchedReduceAddInst implementation#3118
Closed
SplitInfinity wants to merge 1 commit intopytorch:masterfrom
Closed
[OpenCL] Fix bugs in BatchedReduceAddInst implementation#3118SplitInfinity wants to merge 1 commit intopytorch:masterfrom
SplitInfinity wants to merge 1 commit intopytorch:masterfrom
Conversation
Description: This commit fixes two bugs in the OpenCL implementation of BatchedReduceAddInst and adds a few comments for clarity. The first is a segmentation fault caused by incorporating feedback on pytorch#2958. A suggestion was made to make the loop variable i in the loop that computes batchSliceSizes count down instead of count up, but this was done without changing the type (which was size_t, an unsigned type), so the loop never terminates and eventually leads to a segmentation fault. The second bug is an incorrect computation of destSliceSizes. Instead of multiplying the slice size at a dimension with the number of elements in that same dimension, the code was multiplying the former with the number of elements in the *adjacent* dimension. This was surfaced by the unit test added in pytorch#2958 for axis = 2. Testing: 1) ninja check with OpenCL enabled, DEBUG mode 2) ninja check with OpenCL enabled, RELEASE mode 3) ninja check with OpenCL enabled, ASAN+UBSAN mode
nickgg
approved these changes
Jun 14, 2019
facebook-github-bot
left a comment
There was a problem hiding this comment.
@SplitInfinity has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
rdzhabarov
approved these changes
Jun 14, 2019
|
@SplitInfinity merged this pull request in 6609788. |
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 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.
Description
This commit fixes two bugs in the OpenCL implementation of
BatchedReduceAddInstand adds a few comments for clarity.The first is a segmentation fault caused by
incorporating feedback on #2958. A suggestion was made to make the loop
variable
iin the loop that computesbatchSliceSizescount down instead ofcount up, but this suggestion was taken without changing the type (which was
size_t,an unsigned type), so the loop never terminates and eventually leads to a
segmentation fault.
The second bug is an incorrect computation of
destSliceSizes. Instead ofmultiplying the slice size at a dimension with the number of elements in
that same dimension, the code was multiplying the former with the number
of elements in the adjacent dimension. This was surfaced by the unit
test added in #2958 for
axis = 2.Test Plan
ninja checkwith OpenCL enabled, DEBUG modeninja checkwith OpenCL enabled, RELEASE modeninja checkwith OpenCL enabled, ASAN+UBSAN mode