-
Notifications
You must be signed in to change notification settings - Fork 787
[SYCL] Conversion rules application to read/write API in image accessors for host device. #562
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
[SYCL] Conversion rules application to read/write API in image accessors for host device. #562
Conversation
// RUN: clang++ -fsycl %s -o %t.out -lOpenCL | ||
// RUN: env SYCL_DEVICE_TYPE=HOST %t.out | ||
// RUN: %CPU_RUN_PLACEHOLDER %t.out | ||
// RUNx: %GPU_RUN_PLACEHOLDER %t.out |
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.
Issue seen on GPU.
The minimum value written for signed_int8 and signed_int16 datatypes are -127 and -32767 instead of -128 and -32768.
Created a bug for it. All other results read/written are correct. As soon as the bug is fixed, the testing can be enabled.
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.
Ah, interesting they made the choice of symmetry.
host_ptr, image_channel_order::rgba, s::float4(2, -2, 0.375f, 0), | ||
s::float4(2, -2, 0.375f, 0)); | ||
check_read_type_order<s::cl_float4, image_channel_type::fp32>( | ||
host_ptr, image_channel_order::rgba, s::float4(2, -2, 0.375f, 0)); |
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.
Another approach is to use the read API directly after the write API, and pass the ExpectedData of float4/uint4/int4 datatype as a separate argument. This has not been done
to show the difference in the values written to the pixel and read back from the pixel; which also helped in testing and finding bugs.
|
||
// Calling only valid channel types with cl_half4. | ||
// image_channel_type::fp16 | ||
// TODO: Enable the below call. Currently it doesn't work because of half |
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.
Need to enable this after some debugging.
explicit conversion fails when
HostDataPtr[0] == (PixelDataType)ExpectedData.x()
or
HostDataPtr[0] == (WriteDataType)ExpectedData.x() is used in the test.
I removed this need by defining function check_write_data for half PixelDataType at line 57.
The code requires some debugging, which is why it is not enabled yet.
e52b9e5
to
c5db384
Compare
// OpenCL Spec section 6.12.14.2 does not allow reading uint4 data from an | ||
// image with channel datatype other than unsigned_int8,unsigned_int16 and | ||
// unsigned_int32. | ||
assert(!"Datatype of read data - cl_uint4 is incompatible with the " |
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.
Shouldn't it better to make it exception?
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.
Done.
return PixelData; | ||
} | ||
|
||
// Multiplication operator is not supported in vec class for a float operand. | ||
// So here each vector element is individually multiplied by the multiplicand. | ||
#define MULTIPLY_EACH(Result, Operand, Multiplicand) \ |
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.
Suggest converting it to function.
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.
I added it.
Any specific reason for suggesting a function usage here instead of a macro.
|
||
namespace s = cl::sycl; | ||
|
||
using namespace s; |
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.
Why do you "using" namespace s, not sure you are using it.
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.
I'm using it at some lcoations. I will make it consistent.
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.
fixed.
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.
I suggest removing this line.
I remember there were some name conflicts between the symbols from cl::sycl
name space and some other libraries.
It's better to use fully qualified names in the test.
Please, use the alias defined at the line 21.
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.
yes, I saw the error with cl_* scalar types. It gives an error that the datatype has conflicting definitions.
I, therefore didn't use the cl_xx types.
removed using namespace s;
fab6c71
to
ba3bbd6
Compare
switch (ImageChannelType) { | ||
case image_channel_type::snorm_int8: | ||
// max(-1.0f, (float)c / 127.0f) | ||
RetData.x() = (cl_float)PixelData.x() / 127.0f; |
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.
Perhaps you could replace everywhere all the old C-style casts by some static_cast<cl_float>(PixelData.x())
or cl_float { PixelData.x() }
according to the various data types
ba3bbd6
to
6c9d152
Compare
Hi, I have tried to address all the comments. Please let me know if there are any other concerns. |
@romanovvlad @alexeyvoronov @keryell |
I'm rebasing these changes into 2 commits. One for write API and the other for read API. |
6c9d152
to
9929e59
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.
LGTM
|
||
namespace s = cl::sycl; | ||
|
||
using namespace s; |
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.
I suggest removing this line.
I remember there were some name conflicts between the symbols from cl::sycl
name space and some other libraries.
It's better to use fully qualified names in the test.
Please, use the alias defined at the line 21.
8c54c65
to
f0a9029
Compare
f0a9029
to
1504e82
Compare
I hope the code looks better and cleaner now. |
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 a lot better to me.
care of conversion rules as per OpenCL spec 1.2 section 8.3. Addition of image_accessor_write_api.cpp test case. Signed-off-by: Garima Gupta <[email protected]>
1504e82
to
30bc576
Compare
Squashed the last commit. Edited the test case to work with windows. |
Addition of test case for read API without a sampler. Checking is done if the read data is correct and as expected. Signed-off-by: Garima Gupta <[email protected]>
30bc576
to
a6cffe4
Compare
Test case has been added to test the write and read APIs for all valid datatypes-int4,uint4,float4,half4 with valid image_channel_types.
The conversion rules followed are based on opencl spec sec 8.3.
This information is missing in the SYCL spec.