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

Support buffer_location for shared/host USM allocations #141

Merged
merged 2 commits into from
Jun 9, 2022

Conversation

bsyrowik
Copy link
Contributor

@bsyrowik bsyrowik commented Jun 1, 2022

This change adds a new memory property to the aocl_mmd.h file to facilitate passing of buffer location information from the OpenCL runtime to the MMD USM allocation APIs.

The IP authoring project requires this change: USM allocations for memory-mapped host interfaces will have the buffer_location property set, and this should be propagated to the simulation MMD to ensure appropriate allocations are made (e.g. with the correct address bits set).

This complements the following SYCL runtime changes:

@bsyrowik bsyrowik marked this pull request as draft June 1, 2022 21:31
@pcolberg
Copy link
Contributor

pcolberg commented Jun 1, 2022

Thanks Bain! Could you provide a brief motivation of the change in the commit message? Why is the change needed, what flows does it apply to (only IP authoring in simulation?), etc.

@pcolberg pcolberg added the enhancement New feature or request label Jun 1, 2022
@pcolberg pcolberg added this to the 2022.3 milestone Jun 1, 2022
@pcolberg
Copy link
Contributor

pcolberg commented Jun 1, 2022

I suggest we add the runtime implementation to this PR that demonstrates how the new API would work. The idea is to pass the newly received buffer_location property in clSharedMemAllocINTEL() and clHostMemAllocINTEL() through to the MMD using this new attribute, correct? Besides that, is any additional logic needed in these runtime functions?

@bsyrowik
Copy link
Contributor Author

bsyrowik commented Jun 1, 2022

I suggest we add the runtime implementation to this PR that demonstrate how the new API would work. The idea is to pass the newly received buffer_location property in clSharedMemAllocINTEL() and clHostMemAllocINTEL() through to the MMD using this new attribute, correct? Besides that, is any additional logic needed in these runtime functions?

Sounds like a good plan! Yes, you are correct. And no, I don't think there is any additional logic required. :)

@pcolberg pcolberg assigned pcolberg and unassigned pcolberg Jun 1, 2022
@pcolberg pcolberg linked an issue Jun 1, 2022 that may be closed by this pull request
@pcolberg pcolberg changed the title Add new buffer location memory property to aocl_mmd.h Support buffer_location for shared/host USM allocations Jun 1, 2022
@bsyrowik bsyrowik marked this pull request as ready for review June 2, 2022 19:24
@bsyrowik
Copy link
Contributor Author

bsyrowik commented Jun 2, 2022

Alright, I think we're good to go now. @tiwaria1 @pcolberg - please let me know if there are any other changes you'd like :)

@pcolberg
Copy link
Contributor

pcolberg commented Jun 2, 2022

Thanks @bsyrowik, looks good!

For clDeviceMemAllocINTEL(), commit f79a9c2 includes a unit test that queries and verifies the buffer location using clGetMemAllocInfoINTEL(). However, the buffer location is stored within a cl_mem struct, which is populated in clDeviceMemAllocINTEL() but not in clHostMemAllocINTEL() and clSharedMemAllocINTEL(). I wonder if we should add a mem_id field to acl_usm_allocation_t to persist the buffer location for later query?

@pcolberg pcolberg requested review from sophimao and removed request for sophimao June 7, 2022 23:39
@pcolberg pcolberg force-pushed the add_aocl_mmd_buffer_location_mem_prop branch from fe3d0a4 to 43023b2 Compare June 9, 2022 02:45
@pcolberg pcolberg dismissed their stale review June 9, 2022 02:54

Wait for integration testing before approval

@pcolberg
Copy link
Contributor

pcolberg commented Jun 9, 2022

@bsyrowik I have made a slight modification to store the properties in an array on the stack similar to intel/llvm#6220 and intel/llvm#6218. Once this PR passes integration testing, this is ready to be merged.

@pcolberg pcolberg force-pushed the add_aocl_mmd_buffer_location_mem_prop branch from 43023b2 to 9ad15f6 Compare June 9, 2022 17:27
@pcolberg pcolberg merged commit 5255e33 into intel:main Jun 9, 2022
pcolberg added a commit to ericxu233/fpga-runtime-for-opencl that referenced this pull request Jul 20, 2022
A board's MMD may not support passing property lists containing only a
null terminator to aocl_mmd_host_alloc() or aocl_mmd_shared_alloc().

This resolves a regression introducted in intel#141

Signed-off-by: Peter Colberg <peter.colberg@intel.com>
pcolberg added a commit that referenced this pull request Jul 20, 2022
A board's MMD may not support passing property lists containing only a
null terminator to aocl_mmd_host_alloc() or aocl_mmd_shared_alloc().

This resolves a regression introducted in #141

Signed-off-by: Peter Colberg <peter.colberg@intel.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support buffer_location for shared/host USM allocations
4 participants