Device global copy kernel implementation #65

sherry-yuan · 2022-01-24T19:04:42Z

Currently the device global pointer is being mocked by passing an actual device allocation into the function. However this should be removed after the pointer is available through autodiscovery string.

Closes #56

src/acl_mem.cpp

test/acl_mem_test.cpp

artemrad · 2022-02-14T15:28:20Z

What is the purpose of the number field in each at each index of the autodiscovery string?

I would also think about how to add fields containing information about properties associated with each device_global.

sherry-yuan · 2022-02-14T16:42:17Z

Thanks @artemrad for the comment!

What is the purpose of the number field in each at each index of the autodiscovery string?

Autodiscovery format in f89b759: num_dev_global [(num_dev_global_field_1, dev_global_name_1, dev_global_address_1, dev_global_size_1), (num_dev_global_field_2, dev_global_name_2, dev_global_address_2, dev_global_size_2), (...), ...]

By number field you mean the bolded part of above?

The num_dev_global_field keep track of the number of properties associated with each device global. We need this number because the autodiscovery parser relies counting number of remaining item to read per device global. Further, this setup ensure forward compatibility. i.e if we decided to add additional properties in the future for some specific device global, we can easily do so by increasing the num_dev_global_field (which tells the parser there are one more field to read for that specific device global).

If the number of attributes associated with each dev global is always going to be the same, then it probably make more sense to make num_dev_global_field not a per device global number. i.e the format will become

num_dev_global num_dev_global_field [(dev_global_name_1, dev_global_address_1, dev_global_size_1), (dev_global_name_2, dev_global_address_2, dev_global_size_2), (...), ...]

I would also think about how to add fields containing information about properties associated with each device_global.

I think the above mechanism makes sure we can add additional properties for each device global while maintaining forward compatibility? Currently the attributes include dev_global_name, dev_global_address, dev_global_size (there can always be more, let me know if I am missing something). Correct me if I understand attribute wrong.

--------------------------------- Currently the device global pointer is being mocked by passing an actual device allocation into the function. However this should be removed after the pointer is available through autodiscovery string.

…allow it

----------------------------------------- Currently each kernel will receive the info about device global address and size This is potentially not desired, a better design would be to have device global at autodiscovery device level instead, and kernel query for such information during runtime.

---------------------------------------------------------- The autodiscovery specifies how many device global there are in the device Specify for each device global how many attribute it has (currently 3) The 3 attribute are: device global name, address, size The device global name are used in runtime to get address of device global when given the name in clEnqueueReadGlobalVariableINTEL or clEnqueueWriteGlobalVariableINTEL

------------------------------------ Now the autodiscovery format is changed to num_device_global num_field_in_device_global [(dev_global_name, dev_global_addr, devl_global_size), (dev_global_name, dev_global_addr, devl_global_size), ...] This is because each device global will have same number of field anyways, so itis not necessary ot keep a number of field for each device global

sherry-yuan · 2022-03-14T22:22:08Z

Update milestone to 2022.4 as that is the current target.

pcolberg · 2022-04-01T18:54:12Z

src/acl_mem.cpp

+  if (status != CL_SUCCESS) {
+    return status;
+  }
+


You can insert an assert(kernel) after checking the status to appease Klocwork.

pcolberg · 2022-04-01T18:55:20Z

test/acl_usm_test.cpp

+  // Host USM example
+  // void* src_ptr = clHostMemAllocINTEL(m_context, NULL, strsize, 1, &status);
+  // CHECK_EQUAL(CL_SUCCESS, status);
+  // CHECK(src_ptr != NULL);


Commented code should be removed; unless you intend to enable this before merge?

sherry-yuan · 2022-04-04T16:17:46Z

Autodiscovery change is separated out into a separate PR (because the full implementation depend on updating upstream khronos header, which depend on the full implementation to be ready in sycl runtime). Autodiscovery change could be merged first

Autodiscovery change PR: #100

sherry-yuan self-assigned this Jan 24, 2022

sherry-yuan added the enhancement New feature or request label Jan 24, 2022

sherry-yuan added this to the 2022.3 milestone Jan 24, 2022