-
Notifications
You must be signed in to change notification settings - Fork 198
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add testing for CL_KERNEL_LOCAL_MEM_SIZE #1235 #2089
base: main
Are you sure you want to change the base?
Add testing for CL_KERNEL_LOCAL_MEM_SIZE #1235 #2089
Conversation
4e50673
to
301a77f
Compare
|
||
size_t elements = 100; | ||
size_t sizeToAllocate = elements * sizeof(cl_int); | ||
int* localData = (cl_int*)malloc(sizeToAllocate); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use a smart pointer here rather than malloc (e.g. make_unique
) so that if the function returns early before the free(localData)
we don't leak.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
localData
is unused. We may as well remove it entirely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: it seems odd to put this test into test_api_consistency.cpp
, since it has nothing to do with API consistency checks. I'd recommend putting it into its own new file test_kernel_local_memory_size.cpp
, similar to the existing test_kernel_private_memory_size.cpp
.
if (create_single_kernel_helper(context, &program, &kernel, 1, empty_kernel, | ||
"empty_kernel") | ||
!= 0) | ||
{ | ||
return -1; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We really ought to return TEST_FAIL
here rather than the magic number -1
. Or, even better, something like:
if (create_single_kernel_helper(context, &program, &kernel, 1, empty_kernel, | |
"empty_kernel") | |
!= 0) | |
{ | |
return -1; | |
} | |
error = create_single_kernel_helper(context, &program, &kernel, 1, empty_kernel, | |
"empty_kernel"); | |
test_error(error, "Unable to create empty_kernel"); |
@bashbaug I have added some corrections, tested under Intel, Nvidia, POCL and SimpleOpenCLSamples Layers. There were discrepancies in results, I wanted to check all scenarios that get into my mind. Please review and tell me which does not make sense. |
2da26b0
to
7bd8dc0
Compare
3cd581c
to
308b157
Compare
308b157
to
1cace37
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good but noticed that we don't verify the param_value_size_ret
output parameter to clGetKernelWorkGroupInfo
, but a conformant implementation should set that to sizeof(cl_ulong)
. Can we check that too.
81d58cd
to
95b817a
Compare
Discussed in the October 15th teleconference. We will wait one more week for a few more reviews. |
test_assert_error(kernel_local_usage >= memory, | ||
"kernel local mem size failed"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, this check is failing for me for POCL because the queried value is coming back as zero. I'm not sure if this is a POCL issue or if POCL is being clever and optimizing away the local memory array - maybe @pjaaskel can check?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It shouldn't optimize this case away. I opened a PoCL issue. We'll check what's going on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gah, my previous review should have been "request changes", sorry about that.
Here is one more nitpick - there are several misspellings of "verification".
test_assert_error(local_memory_kernel_verify(), | ||
"local_memory_kernel data verificaion failed"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test_assert_error(local_memory_kernel_verify(), | |
"local_memory_kernel data verificaion failed"); | |
test_assert_error(local_memory_kernel_verify(), | |
"local_memory_kernel data verification failed"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are still two additional places with the same misspelling.
|
||
return true; | ||
}; | ||
test_assert_error( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be changed to:
auto local_param_local_memory_kernel_verify = & {
constexpr size_t size = 10;
int testData[size];
for (size_t i = 0; i < size; i++)
{
testData[i] = i * 2;
}
int temp = testData[9];
for (size_t i = 0; i < size; i++)
{
if (i == 9)
testData[9] += temp;
else
testData[9] += testData[i];
}
testData[9] += 666;
for (size_t i = 0; i < size; i++)
{
if (data[i] != testData[i]) return false;
}
return true;
};
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good in present form, suggested a modification.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I'm OK with this going in as-is.
The POCL failure still exists with the latest code, though this may be a legitimate POCL failure.
#1235