Skip to content

[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

Merged
merged 2 commits into from
Sep 19, 2019

Conversation

garimagu
Copy link
Contributor

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.

// 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
Copy link
Contributor Author

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.

Copy link
Contributor

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));
Copy link
Contributor Author

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
Copy link
Contributor Author

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.

@garimagu garimagu force-pushed the private/garimagu/readAPI_imageAccessors2 branch from e52b9e5 to c5db384 Compare August 28, 2019 00:12
@bader bader changed the title Conversion rules application to read/write API in image accessors for host device. [SYCL] Conversion rules application to read/write API in image accessors for host device. Aug 28, 2019
// 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 "
Copy link
Contributor

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?

Copy link
Contributor Author

@garimagu garimagu Aug 30, 2019

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) \
Copy link
Contributor

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.

Copy link
Contributor Author

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;
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed.

Copy link
Contributor

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.

Copy link
Contributor Author

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;

@garimagu garimagu force-pushed the private/garimagu/readAPI_imageAccessors2 branch 2 times, most recently from fab6c71 to ba3bbd6 Compare August 31, 2019 00:09
switch (ImageChannelType) {
case image_channel_type::snorm_int8:
// max(-1.0f, (float)c / 127.0f)
RetData.x() = (cl_float)PixelData.x() / 127.0f;
Copy link
Contributor

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

@garimagu garimagu force-pushed the private/garimagu/readAPI_imageAccessors2 branch from ba3bbd6 to 6c9d152 Compare September 4, 2019 01:58
@garimagu
Copy link
Contributor Author

garimagu commented Sep 4, 2019

Hi, I have tried to address all the comments. Please let me know if there are any other concerns.
I'm yet to enable the test case for half datatype.

@garimagu
Copy link
Contributor Author

garimagu commented Sep 4, 2019

@romanovvlad @alexeyvoronov @keryell
Let me know if you feel some other changes are needed.

@garimagu
Copy link
Contributor Author

garimagu commented Sep 4, 2019

I'm rebasing these changes into 2 commits. One for write API and the other for read API.
It will be great to have some progress on this review so that I can upload further reviews that depend on this change.

Copy link
Contributor

@romanovvlad romanovvlad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

bader
bader previously requested changes Sep 11, 2019

namespace s = cl::sycl;

using namespace s;
Copy link
Contributor

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.

@garimagu garimagu force-pushed the private/garimagu/readAPI_imageAccessors2 branch 2 times, most recently from 8c54c65 to f0a9029 Compare September 13, 2019 00:44
@garimagu garimagu force-pushed the private/garimagu/readAPI_imageAccessors2 branch from f0a9029 to 1504e82 Compare September 16, 2019 17:21
@garimagu
Copy link
Contributor Author

I hope the code looks better and cleaner now.
Thanks for the comments.

Copy link
Contributor

@alexeyvoronov-intel alexeyvoronov-intel left a 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]>
@garimagu
Copy link
Contributor Author

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]>
@garimagu garimagu force-pushed the private/garimagu/readAPI_imageAccessors2 branch from 30bc576 to a6cffe4 Compare September 18, 2019 01:38
@bader bader dismissed their stale review September 19, 2019 14:45

Changes applied.

@bader bader merged commit 8c7d0e9 into intel:sycl Sep 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants