Skip to content

[SYCL] Image_accessor Host Implementation #271

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 1 commit into from
Jul 12, 2019
Merged

Conversation

garimagu
Copy link
Contributor

  • Enabled get_access methods in image class.
  • Implemented base class for accessor to image/image_array/host_image
    access target for host compiler.
  • Implemented a few APIs for Image accessors.
  • Test code to be added after scheduler support is added for image
    class.

Signed-off-by: Garima Gupta garima.gupta@intel.com

Copy link
Contributor

@keryell keryell left a comment

Choose a reason for hiding this comment

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

It is not clear that there is some concrete implementation as promised in the commit title.
Also a few remarks.

static_assert(
AccessMode == access::mode::read || AccessMode == access::mode::write ||
AccessMode == access::mode::discard_write,
"Access modes can be read/write/discard_write for image accessor.");
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"Access modes can be read/write/discard_write for image accessor.");
"Access modes can be only read/write/discard_write for image accessor.");

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

"Dimensions can be 1/2/3 for image accessor.");

template <bool B, class T = void>
using enable_if_t = typename std::enable_if<B, T>::type;
Copy link
Contributor

Choose a reason for hiding this comment

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

It has been centralized in detail:: now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately, it's not done yet.
This refactoring is a part of #221, which is not merged yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So i'll keep the code as is then? We can do the change once #221 is checked in.

Copy link
Contributor

Choose a reason for hiding this comment

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

#221 is checked in, so please, re-use existing alias.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

typename = enable_if_t<(Dims > 0 && Dims <= 3) && IsHostImageAcc>>
image_accessor(image<Dims, AllocatorT> &ImageRef, int ImageElementSize)
#ifdef __SYCL_DEVICE_ONLY__
{ // No constructor needed for the device side.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is actually a constructor you are defining here...
Do you mean = delete; instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

Or just change the comment to fit the goal.

Copy link
Contributor

Choose a reason for hiding this comment

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

I had similar issue with buffer accessor few months ago, removing '{ }' did not work because the constructor was needed even for DEVICE because the filtering of DEVICE code happened much later.
I don't remember the details.
Garima, did you try " = delete", or having ';' instead of '{ }' ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It reports the error when i try to use instantiate it.
error: call to deleted constructor of 'image_accessor

@garimagu
Copy link
Contributor Author

garimagu commented Jun 29, 2019

It is not clear that there is some concrete implementation as promised in the commit title.
Also a few remarks. 👍

I'll edit the commit message to be more adherent to the code. You are right.

@garimagu
Copy link
Contributor Author

garimagu commented Jul 5, 2019

Let me know if something else is needed for this review.

v-klochkov
v-klochkov previously approved these changes Jul 9, 2019
bader
bader previously requested changes Jul 12, 2019
APIs.
- Enabled get_access methods in image class.
- Base class for accessor to image/image_array/host_image
  access target for host compiler introduced with basic implementation.
- Implemented a few APIs for Image accessors for host compiler - get_count
  and get_size.
- Test code to be added after scheduler support is added for image
  class.

Signed-off-by: Garima Gupta <garima.gupta@intel.com>
@bader bader dismissed their stale review July 12, 2019 18:05

All my issues were resolved, but I'd like @romanovvlad to approve before merging.

@bader bader merged commit e58c84d into intel:sycl Jul 12, 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