Fix the issue when NHWC Tensor has height or width larger then max cuda grid #28931
Conversation
ifedan
changed the title
| maxThreadsDim[0], std::min<int>(lastPow2(nInputPlane), max_threads / block_y / block_z)); | ||
| const dim3 block(block_x, block_y, block_z); | ||
| int grid_x = nbatch; | ||
| int grid_y = cuda::ATenCeilDiv(safe_downcast<int, int64_t>(outputWidth), block_y*BLOCK_STRIDE); |
There was a problem hiding this comment.
We shouldn't need multiple kernel launching, as we are striding along width/height in the kernel already.
I would discard all changes and simply add change this line (and grid_z as well)
int grid_y = std::min<int>(
at::cuda::getCurrentDeviceProperties()->maxGridSize[1],
cuda::ATenCeilDiv(safe_downcast<int, int64_t>(outputWidth), block_y*BLOCK_STRIDE));
Please do NOT do the same thing for grid_x, as the kernel does not loop over batch dimension. The fix you have here could be used for that.
There was a problem hiding this comment.
I updated grid, and removed slicing, but I still see the performance degradation for backward function.
There was a problem hiding this comment.
hmmm, perf regression doesn't really make any sense here. It's basically the same kernel.
Not sure if it's related to that typo I pointed out in the other comment.
Changing the grid size would affect the occupancy and maybe cache hit? I need to think about it further before I can make any decisive call. But let's fix the typo and hope the problem goes away
| maxThreadsDim[0], std::min<int>(lastPow2(nInputPlane), max_threads / block_y / block_z)); | ||
| const dim3 block(block_x, block_y, block_z); | ||
| int grid_x = nbatch; | ||
| int grid_y = cuda::ATenCeilDiv(safe_downcast<int, int64_t>(inputWidth), block_y*BLOCK_STRIDE); |
There was a problem hiding this comment.
Same thing as I commented on forward.
| cuda::ATenCeilDiv(safe_downcast<int, int64_t>(outputWidth), block_y*BLOCK_STRIDE)); | ||
| int grid_z = std::min<int>( | ||
| at::cuda::getCurrentDeviceProperties()->maxGridSize[2], | ||
| cuda::ATenCeilDiv(safe_downcast<int, int64_t>(outputHeight), block_y*BLOCK_STRIDE)); |
There was a problem hiding this comment.
Typo here! block_y should be block_z
| cuda::ATenCeilDiv(safe_downcast<int, int64_t>(inputWidth), block_y*BLOCK_STRIDE)); | ||
| int grid_z = std::min<int>( | ||
| at::cuda::getCurrentDeviceProperties()->maxGridSize[2], | ||
| cuda::ATenCeilDiv(safe_downcast<int, int64_t>(inputHeight), block_y*BLOCK_STRIDE)); |
|
|
ifedan
changed the title
|
LGTM. Did that few line change actually give a hit on performance? Or is there a high variance between runs? |
facebook-github-bot
left a comment
facebook-github-bot
left a comment
There was a problem hiding this comment.
@ifedan has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
…da grid (#28931) Summary: When NHWC Tensor has height or width larger then max CUDA grid size, max_pool fails with error code 0 The example is: pytorch/pytorch#28714 This change should limit grid size to the CUDA max possible size and chunk the input to be able to process it. Pull Request resolved: pytorch/pytorch#28931 Differential Revision: D18358892 Pulled By: ifedan fbshipit-source-id: 2fd65448bd644f1588a0e208edaaea5bcb6a7d52
5 participants
When NHWC Tensor has height or width larger then max CUDA grid size, max_pool fails with error code 0
The example is: #28714
This change should limit grid size to the CUDA max possible size and chunk the input to be able to process it.