[HLSL][QC] Modify stride on arrays-16bit.test#754
[HLSL][QC] Modify stride on arrays-16bit.test#754joaosaffran wants to merge 11 commits intollvm:mainfrom
Conversation
joaosaffran
added
the
test-all
joaosaffran
force-pushed
the
bugfix/qc-arrays-16bit
branch
from
d797d08 to
8234eb5
Compare
| uint16_t c1[2][2]; | ||
| float16_t c2[1]; | ||
| uint16_t4 c3[2]; | ||
| uint16_t c4[1]; |
There was a problem hiding this comment.
This is doing more than modifying the stride?
Is this working around some cbuffer layout issue?
| # add a small amount of buffer at the end here. | ||
| Stride: 28 | ||
| FillSize: 28 | ||
| FillSize: 64 |
There was a problem hiding this comment.
Why isn't the fillsize still 28?
There was a problem hiding this comment.
When running the Vulkan validation layer, it is outputting the following error:
Validation Error: [ VUID-VkMappedMemoryRange-size-01389 ] | MessageID = 0xee4872d
vkFlushMappedMemoryRanges(): pMemoryRanges[0].size is VK_WHOLE_SIZE and the mapping end (28 = 0 + 28) not a multiple of VkPhysicalDeviceLimits::nonCoherentAtomSize (64) and not equal to the end of the memory object (32).
The Vulkan spec states: If size is equal to VK_WHOLE_SIZE, the end of the current mapping of memory must either be a multiple of VkPhysicalDeviceLimits::nonCoherentAtomSize bytes from the beginning of the memory object, or be equal to the end of the memory object (https://docs.vulkan.org/spec/latest/chapters/memory.html#VUID-VkMappedMemoryRange-size-01389)
Objects: 1
[0] VkDeviceMemory 0x80000000008
To fix that, I need to modify the fill size to match 32 or 64. That causes the extra padding in the output below.
There was a problem hiding this comment.
I think that maybe rules like this should be enforced in the C++ code. For example, something like this is done already for D3D in constant buffers -
offload-test-suite/lib/API/DX/Device.cpp
Line 848 in b2d1e70
| 0x0005, 0x0006, 0x0007, 0x0008, | ||
| 0x0, | ||
| 0x0009, | ||
| 0x0000, 0x0000, 0x0000, 0x0000, |
There was a problem hiding this comment.
why is there all this extra padding now? because of the larger fill size?
|
this also didn't run any of the tests... |
| 0x3c00, 0x5A5A, 0x5A5A, 0x5A5A, 0x5A5A, 0x5A5A, 0x5A5A, 0x5A5A, | ||
| 0x0001, 0x0002, 0x0003, 0x0004, 0x5A5A, 0x5A5A, 0x5A5A, 0x5A5A, | ||
| 0x0005, 0x0006, 0x0007, 0x0008, 0x5A5A, 0x5A5A, 0x5A5A, 0x5A5A, | ||
| 0x0009, 0x5A5A, 0x5A5A, 0x5A5A, 0x5A5A, 0x5A5A, 0x5A5A, 0x5A5A, |
There was a problem hiding this comment.
Should this last line be here? This is beyond the end of the data covered by CBArrays, isn't it?
If we need to pad this buffer to be a multiple of something then we should make the whole thing be 0x5A5As to be consistent with the values used for the other padding in this test.
There was a problem hiding this comment.
I'd like to understand why we need to add the extra padding here. It looks like this is to round it up to an 8-byte boundary?
| # add a small amount of buffer at the end here. | ||
| Stride: 28 | ||
| FillSize: 28 | ||
| FillSize: 32 |
There was a problem hiding this comment.
it sounds like @damyanp suggestion of fixing this issue in the C++ would be preferable because other tests might have this problem and it isn't clear what the padding is for.
|
I will close this issue, since this is a offloader bug |
By issue you mean PR? the stride still needs to be fixed right? |
4 participants
This patch is modifying the stride for the arrays-16bit.test, in order to fix the vulkan validation errors raised by validation layer.