KhronosGroup / KhronosGroup/OpenCL-CTS
Clean up slice padding logic in clCopyImage
- Dominant language
- C++
- Stars
- 232
- Forks
- 235
- Avg merge
- 8d 7h
- Merged PRs (30d)
- 18
Description
From #1258 review comment https://github.com/KhronosGroup/OpenCL-CTS/pull/1258/files#r696781507, referring to the `mappedSlicedPad` computation in `create_image`:
> this whole construction is very awkward since mappedSlicePad is also wrong for 1D images (since mappedSlice must be 0 for 1D). It is used two different ways: to determine if the whole image can be copied in one go, and to implement the line-by-line copy for 2Darray and 3D images only. This requires extra logic in the if condition of line 247 and the (incomplete) comment in line 275 to mention that it's wrong for 2D images:
> // mappedSlicePad is incorrect for 2D images here, but we will exit the z loop before this is a problem.
> The comment doesn't mention that it's also wrong for 1D and 1Darray images as well.
>
> Although this fix works, I'd propose cleaning up a bit to implement a boolean called something like isMappedSlicePadded, which is set to true only for slice-padded 2Darray or 3D images. Then we can get rid of the (mappedSlicePad==0 || (imageInfo->depth==0 && imageInfo->arraySize==0)) expression below and replace it with !isMappedSlicedPadded. Since the numeric value of mappedSlicePad is only used in the line-by-line copy loop, we could simply do the calculation in the loop and note that it's computing slice padding for 2Darray and 3D images.
Should check other tests for similar logic and provide a common fix.
Contributor guide
No contributing guide indexed for this repository
Research direction
Start at the create_image logic used by clCopyImage and trace mappedSlicedPad through the whole-image and line-by-line copy paths. Check other tests for similar slice-padding logic, then verify that the cleanup preserves copying for 1D, 1D-array, 2D, 2D-array, and 3D images.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp
- Domain
- testing-qa
- Issue type
- Refactor
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100