Skip to content
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

ur: add enqueue fill image #50

Open
veselypeta opened this issue Nov 24, 2022 · 7 comments
Open

ur: add enqueue fill image #50

veselypeta opened this issue Nov 24, 2022 · 7 comments
Assignees
Labels
images UR images memory Memory allocations/transfers/operations specification Changes or additions to the specification
Milestone

Comments

@veselypeta
Copy link
Contributor

PI currently has an API function piEnqueueMemImageFill which is analogous to the openCL version clEnqueueFillImage

We should add this to Unified Runtime

@veselypeta veselypeta self-assigned this Nov 24, 2022
@veselypeta
Copy link
Contributor Author

Turns out this is already added to the spec, but does not appear in the generated header since it's not supported by level-zero
https://github.com/oneapi-src/unified-runtime/blob/main/scripts/core/enqueue.yml#L675

@kbenzie kbenzie added the needs-discussion This needs further discussion label Nov 24, 2022
@kbenzie
Copy link
Contributor

kbenzie commented Nov 24, 2022

We should discuss why there are disabled entry points with the WG.

@kbenzie kbenzie reopened this Nov 24, 2022
@kbenzie kbenzie changed the title ur: add enqueu fill image ur: add enqueue fill image Nov 24, 2022
@kbenzie
Copy link
Contributor

kbenzie commented Nov 24, 2022

Along with urEnqueueMemImageFill, urEnqueueMemImageMap is also currently disabled.

@kbenzie kbenzie added pi DPC++ PI requirement memory Memory allocations/transfers/operations labels Dec 5, 2022
@kbenzie kbenzie added images UR images specification Changes or additions to the specification and removed pi DPC++ PI requirement labels Feb 9, 2023
@veselypeta
Copy link
Contributor Author

  • piEnqueueMemImageFill is only implemented in the OpenCL plugin
  • piEnqueueMemImageMap does not exist it PI.

I suppose we do not need to add these entry-points

@kbenzie
Copy link
Contributor

kbenzie commented May 2, 2023

If these are necessary to implement SYCL 2020, they should be added to the UR spec.

@kbenzie kbenzie removed the needs-discussion This needs further discussion label May 2, 2023
@kbenzie
Copy link
Contributor

kbenzie commented May 5, 2023

Based on discussion with @AerialMantis it does not appear that these commands are required for SYCL 2020 because it is only possible to read() and write() images via the unsampled and sampled image accessors.

Resolution of this issue, should be to remove the currently disabled urMemImageFill entry point from the yaml file.

@kbenzie kbenzie assigned kbenzie and veselypeta and unassigned veselypeta May 5, 2023
@kbenzie kbenzie closed this as completed May 10, 2023
@kbenzie kbenzie added this to the 0.7 milestone Aug 3, 2023
aarongreig added a commit to aarongreig/unified-runtime that referenced this issue Nov 22, 2023
Previously these were "hidden" entry points excluded from generating any
code by setting their versions to "9999.0". Issue oneapi-src#50 goes into detail
about why this was done, and concludes that the entry points should be
removed but we never got around to it at the time.
@aarongreig
Copy link
Contributor

These entry points are still in the spec files. I would have already opened the PR to get them deleted but strangely MemImageFill does appear to get used in the sycl runtime: https://github.com/intel/llvm/blob/sycl/sycl/source/detail/memory_manager.cpp#L851 and not just in a sort of stubbed out helper function or whatever.

I think what's happening here is that SYCL has this fill function that takes an accessor, which is a potentially multi-dimensional array view type object that sits on top of an allocation. You can see there's a little bit of extra code where I linked in the "not image" branch that deals with it possibly being a > 1D operation for buffers. It looks like in the "yes image" branch they're taking advantage of the fact that there's this magic API for images that deals with that nonsense for them, it seems more like a use out of convenience than out of necessity.

Technically I think we could implement ImageFill for all the adapters that support images, but for everything that isn't opencl it would basically be a regular fill with the additional faff of keeping track of which mem_obj belongs to which image and figuring out what our pixel size and strides etc are. All that seems kind of pointless when that one use that I linked has that info on hand and leaves it out when it calls the ImageFill entry point instead of the BufferFill one.

Anyway we should probably bring this up to sycl maintainer people because if we delete ImageFill now we should also figure out how we can delete it from PI and memory_manager so it doesn't come back to bite us later when it's time to port.

@aarongreig aarongreig reopened this Nov 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
images UR images memory Memory allocations/transfers/operations specification Changes or additions to the specification
Projects
None yet
Development

No branches or pull requests

3 participants