[CUDA][HIP] Fix kernel arguments being overwritten when added out of order #2559
Conversation
fabiomestre
force-pushed
the
fix_kernel_arg_indices
branch
from
4400095 to
7ca2133
Compare
fabiomestre
force-pushed
the
fix_kernel_arg_indices
branch
3 times, most recently
from
ccccf85 to
0a071be
Compare
fabiomestre
changed the title
There was a problem hiding this comment.
Could we add CTS tests for this? Probably in test/conformance/exp_command_buffer/update/local_memory_update.cpp & test/conformance/kernel/urKernelSetArgLocal.cpp
Might make it easier if we want to merge this fix earlier than the intel/llvm#16573 feature
fabiomestre
force-pushed
the
fix_kernel_arg_indices
branch
from
86885a1 to
bf6b6f9
Compare
source/adapters/cuda/kernel.hpp
Outdated
| if (ParamSizes[Index] == 0) { | ||
| ParamSizes[Index] = Size; | ||
| std::memcpy(&Storage[InsertPos], Arg, Size); | ||
| Indices[Index] = &Storage[InsertPos]; |
There was a problem hiding this comment.
This isn't an issue introduced by this PR, but the description of Indices as Byte offset into /p Storage allocation for each parameter. is a little confusing, isn't it? An index, to me, implies a relative value from the beginning of the storage, not an absolute address. Do you think we should do something about this? I note this PR is embedding the use of Indices more deeply into the code so it might be best to fix now.
There was a problem hiding this comment.
Yes, I don't mind changing the variable name and related getters. Do you have a preference for the name? Perhaps ParamPointers or ParamOffsets `?
There was a problem hiding this comment.
The existing getter was called getArgIndices() so I decided to rename the variable to ArgPointers and the getter to getArgPointers(). Hopefully that name makes more sense.
source/adapters/hip/kernel.hpp
Outdated
| // For every existing local argument which follows at later argument | ||
| // indices, update the offset and pointer into the kernel local memory. | ||
| // Required as padding will need to be recalculated. | ||
| // Iterate over all existing local argument which follows StartIndex |
There was a problem hiding this comment.
| // Iterate over all existing local argument which follows StartIndex | |
| // Iterate over all existing local arguments which follow StartIndex |
| // Iterate over all existing local argument which follows StartIndex | |
| // Iterate over each existing local argument which follows StartIndex |
either works
fabiomestre
changed the title
| void TearDown() override { | ||
| if (command_handle) { | ||
| EXPECT_SUCCESS(urCommandBufferReleaseCommandExp(command_handle)); | ||
| } | ||
|
|
||
| UUR_RETURN_ON_FATAL_FAILURE( | ||
| LocalMemoryUpdateTestBaseOutOfOrder::TearDown()); | ||
| } |
There was a problem hiding this comment.
Nothing to do right now, but heads up this will conflict with #2578
In the Cuda and Hip adapter, when kernel arguments are added out of order (e.g. argument at index 1 is added before argument at index 0), the existing arguments are currently being overwritten. This happens because some of the argument sizes might not be known when adding them out of order and the code relies on those sizes to choose where to store the argument. This commit avoids this issue by storing the arguments in the same order that they are added and accessing them using pointer offsets.
github-actions
bot
added
the
command-buffer
fabiomestre
force-pushed
the
fix_kernel_arg_indices
branch
from
27a3561 to
e3dcfc3
Compare
In the Cuda and Hip adapter, when kernel arguments are added out of order (e.g. argument at index 1 is added before argument at index 0), the existing arguments are currently being overwritten. This happens because some of the argument sizes might not be known when adding them out of order and the code relies on those sizes to choose where to store the argument.
This PR avoids this issue by storing the arguments in the same order that they are added and accessing them using pointer offsets.