Fix segmentation fault and calculation error in AveragePool2dKernel #2091
Conversation
|
There was a problem hiding this comment.
Pull Request Overview
This PR fixes critical issues in the AveragePool2dKernel implementation for XPU devices, specifically addressing a segmentation fault and calculation error that were causing test failures.
- Replaced direct index access with XPU_KERNEL_LOOP macros for safer kernel iteration
- Changed data types from index_t to int64_t for better overflow handling
- Added group_size limit to prevent exceeding hardware capabilities
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| const int64_t height, | ||
| const int64_t width, | ||
| const int64_t pooled_height, | ||
| const int pooled_width, |
There was a problem hiding this comment.
Inconsistent data type usage: pooled_width uses int while other dimension parameters use int64_t. This inconsistency could lead to overflow issues or unexpected behavior when dealing with large tensors. Consider changing to const int64_t pooled_width for consistency.
Copilot uses AI. Check for mistakes.
| const int64_t height_; | ||
| const int64_t width_; | ||
| const int64_t pooled_height_; | ||
| const int pooled_width_; |
There was a problem hiding this comment.
Inconsistent data type in member variable: pooled_width_ uses int while other dimension member variables use int64_t. This should be const int64_t pooled_width_ to match the constructor parameter and prevent potential overflow issues.
Copilot uses AI. Check for mistakes.
| const uint32_t group_size = | ||
| std::min(static_cast<int>(syclMaxWorkItemsPerSubSlice()), 1024); |
There was a problem hiding this comment.
This hard-code may have a negative impact on some platforms, especially future platforms.
There was a problem hiding this comment.
May I know why here need 1024?
Fixed the following issues found by test/test_nn.py::TestNNDeviceTypeXPU::test_avg_pool_large_tensor2_xpu