[UR] Proposal of new API for memory object properties #571

KseniyaTikhomirova · 2023-06-06T11:15:58Z

No description provided.

Signed-off-by: Tikhomirova, Kseniya <kseniya.tikhomirova@intel.com>

kbenzie

These files are automatically generated from a YAML description of the API. Please read the Contributing Guide for infomation about how to go about making changes.

This reverts commit a5c6888.

Signed-off-by: Tikhomirova, Kseniya <kseniya.tikhomirova@intel.com>

KseniyaTikhomirova · 2023-06-06T15:00:09Z

These files are automatically generated from a YAML description of the API. Please read the Contributing Guide for infomation about how to go about making changes.

Thanks for pointing to that, updated.

kbenzie

Our script has hidden an error, I'm working on a fix for this bug.

In the file scripts/core/PROG.rst in the Kernels section, the example code should be updated to match the entry point as defined in the spec.

scripts/core/kernel.yml

kbenzie · 2023-06-07T13:37:33Z

Our script has hidden an error, I'm working on a fix for this bug.

#580 has fixed this bug. Please rebase and rerun the generate target to work through the errors.

Signed-off-by: Tikhomirova, Kseniya <kseniya.tikhomirova@intel.com>

KseniyaTikhomirova · 2023-06-07T16:10:19Z

Our script has hidden an error, I'm working on a fix for this bug.

#580 has fixed this bug. Please rebase and rerun the generate target to work through the errors.

done!

jandres742 · 2023-06-07T18:53:44Z

scripts/core/kernel.yml

    - $X_RESULT_ERROR_INVALID_KERNEL_ARGUMENT_INDEX
 --- #--------------------------------------------------------------------------
+type: struct
+desc: "Properties for for $xKernelSetArgMemObj."


L0 doesnt set these memory properties per kernel, but per object. Do we have that functionality in CUDA or other adapters?

@jandres742 AFAIK L0 plugin is intended to use access mode to get a proper memory handle (per object) to set it as exact kernel argument (per kernel). Other plugins now just ignore this property.
What L0 API you mentioned?

The closest L0 has is:
https://spec.oneapi.io/level-zero/latest/core/api.html?highlight=mostly#_CPPv418ze_memory_advice_t

but not WRITE_ONLY, READ_ONLY, READ_WRITE.

AFAIK L0 plugin is intended to use access mode to get a proper memory handle (per object) to set it as exact kernel argument (per kernel).

could you elaborate on that? How the L0 plugin gets the memory handle based on access mode?

enumerator ZE_MEMORY_ADVICE_SET_READ_MOSTLY hint that memory will be read from frequently and written to rarely enumerator ZE_MEMORY_ADVICE_CLEAR_READ_MOSTLY removes the affect of [ZE_MEMORY_ADVICE_SET_READ_MOSTLY](https://spec.oneapi.io/level-zero/latest/core/api.html?highlight=mostly#ze__api_8h_1a54593bb72c476f6c3ac90909ae126cdfaf2a7ce987fb033859d05bc5dd812b1f5) enumerator ZE_MEMORY_ADVICE_SET_PREFERRED_LOCATION hint that the preferred memory location is the specified device enumerator ZE_MEMORY_ADVICE_CLEAR_PREFERRED_LOCATION removes the affect of [ZE_MEMORY_ADVICE_SET_PREFERRED_LOCATION](https://spec.oneapi.io/level-zero/latest/core/api.html?highlight=mostly#ze__api_8h_1a54593bb72c476f6c3ac90909ae126cdfaf56067d28d73977bb30cc6a35d89520c) enumerator ZE_MEMORY_ADVICE_SET_NON_ATOMIC_MOSTLY hints that memory will mostly be accessed non-atomically enumerator ZE_MEMORY_ADVICE_CLEAR_NON_ATOMIC_MOSTLY removes the affect of [ZE_MEMORY_ADVICE_SET_NON_ATOMIC_MOSTLY](https://spec.oneapi.io/level-zero/latest/core/api.html?highlight=mostly#ze__api_8h_1a54593bb72c476f6c3ac90909ae126cdfa62a34e1d7d43897cd3d9318474ba88fd) enumerator ZE_MEMORY_ADVICE_BIAS_CACHED hints that memory should be cached enumerator ZE_MEMORY_ADVICE_BIAS_UNCACHED hints that memory should be not be cached enumerator ZE_MEMORY_ADVICE_FORCE_UINT32[¶](https://spec.oneapi.io/level-zero/latest/core/api.html?highlight=mostly#_CPPv4N18ze_memory_advice_t29ZE_MEMORY_ADVICE_FORCE_UINT32E)

clarified with @smaslov-intel and yes, we are actually already implementing this in L0 adapter.

@kbenzie : about the advantages: if the argument is only R, we avoid having extra copies needed to update the argument if it were RW or W, so there's actually advantages on doing this. @smaslov-intel would correct me.

Okay cool. It also feels like other adapters could potentially also make use of this information if/when they support multiple devices per context.

kbenzie · 2023-06-09T12:15:16Z

Before we actually make this change, does this actaully produce a measurable performance improvement?

kbenzie

I think the API change is looking fine. There is still an open question about the benefits of this change.

KseniyaTikhomirova · 2023-06-12T08:50:47Z

I think the API change is looking fine. There is still an open question about the benefits of this change.

lets wait for @smaslov-intel who submitted this request to SYCL RT.

smaslov-intel · 2023-06-12T13:43:39Z

I think the API change is looking fine. There is still an open question about the benefits of this change.

lets wait for @smaslov-intel who submitted this request to SYCL RT.

I had requested this to optimize SYCL buffer accesses in context with multiple devices. Currently we have to assume that buffer accesses from kernel are always read-write https://github.com/intel/llvm/blob/3024161f1b7b213dc0c5e6c0e879acb0fb83205f/sycl/plugins/unified_runtime/ur/adapters/level_zero/ur_level_zero_kernel.cpp#L683, which invalidates memory representing the buffer on different devices, resulting in redundant copies. They can take noticeable time for large buffers, but I don't have any measured benefits.

KseniyaTikhomirova · 2023-06-13T14:49:35Z

@kbenzie, @jandres742, are you ok to proceed with the new API based on the explanation above?
(I still need to rebase the patch but want to ensure that we have an agreement).

jandres742 · 2023-06-13T14:58:31Z

I think the API change is looking fine. There is still an open question about the benefits of this change.

lets wait for @smaslov-intel who submitted this request to SYCL RT.

I had requested this to optimize SYCL buffer accesses in context with multiple devices. Currently we have to assume that buffer accesses from kernel are always read-write https://github.com/intel/llvm/blob/3024161f1b7b213dc0c5e6c0e879acb0fb83205f/sycl/plugins/unified_runtime/ur/adapters/level_zero/ur_level_zero_kernel.cpp#L683, which invalidates memory representing the buffer on different devices, resulting in redundant copies. They can take noticeable time for large buffers, but I don't have any measured benefits.

@smaslov-intel : I'm not following how the adapters (at least L0) would provide support for this. Could you elaborate? is there something we need to add to L0? does the support exist already on OCL or CUDA?

smaslov-intel · 2023-06-13T16:13:25Z

I think the API change is looking fine. There is still an open question about the benefits of this change.

lets wait for @smaslov-intel who submitted this request to SYCL RT.

I had requested this to optimize SYCL buffer accesses in context with multiple devices. Currently we have to assume that buffer accesses from kernel are always read-write https://github.com/intel/llvm/blob/3024161f1b7b213dc0c5e6c0e879acb0fb83205f/sycl/plugins/unified_runtime/ur/adapters/level_zero/ur_level_zero_kernel.cpp#L683, which invalidates memory representing the buffer on different devices, resulting in redundant copies. They can take noticeable time for large buffers, but I don't have any measured benefits.

@smaslov-intel : I'm not following how the adapters (at least L0) would provide support for this. Could you elaborate? is there something we need to add to L0? does the support exist already on OCL or CUDA?

@jandres742 : The link that I provided above is how we (start) handling a buffer migration (it is inside L0 adapter/plugin), Feel free to contact me for details

veselypeta

LGMT. But can you ensure that all the kernel tests still compile.

scripts/core/kernel.yml

Signed-off-by: Tikhomirova, Kseniya <kseniya.tikhomirova@intel.com>

KseniyaTikhomirova · 2023-06-14T12:29:07Z

@kbenzie Hi, I am trying to rebase changes to solve merge conflicts and script for spec generation silently fails now. It also fails on main branch without my changes. Is it something known? Could you advise how to solve it?
Generating HTML...
Failed to generate specification.
gmake[7]: *** [CMakeFiles/generate-code] Error 1
gmake[7]: Leaving directory /localdisk2/ktikhomi/repo/unified-runtime/build' gmake[6]: *** [CMakeFiles/generate-code.dir/all] Error 2 gmake[6]: Leaving directory /localdisk2/ktikhomi/repo/unified-runtime/build'
gmake[5]: *** [CMakeFiles/generate-code.dir/rule] Error 2
gmake[5]: Leaving directory /localdisk2/ktikhomi/repo/unified-runtime/build' gmake[4]: *** [generate-code] Error 2 gmake[4]: Leaving directory /localdisk2/ktikhomi/repo/unified-runtime/build'
make[3]: *** [CMakeFiles/generate.dir/build.make:57: CMakeFiles/generate] Error 2
make[3]: Leaving directory '/localdisk2/ktikhomi/repo/unified-runtime/build'
make[2]: *** [CMakeFiles/Makefile2:599: CMakeFiles/generate.dir/all] Error 2
make[2]: Leaving directory '/localdisk2/ktikhomi/repo/unified-runtime/build'
make[1]: *** [CMakeFiles/Makefile2:606: CMakeFiles/generate.dir/rule] Error 2
make[1]: Leaving directory '/localdisk2/ktikhomi/repo/unified-runtime/build'
make: *** [Makefile:409: generate] Error 2

kbenzie · 2023-06-14T13:35:07Z

@kbenzie Hi, I am trying to rebase changes to solve merge conflicts and script for spec generation silently fails now. It also fails on main branch without my changes. Is it something known? Could you advise how to solve it?

I just checkout out your branch and successfully built it locally, so I think its a local configuration hiccup you have.

My hunch is that your Python virtual environment was not enabled when make generate was invoked because I don't see any lines that look like this (from my local build):

Generating HTML...
sphinx-build -M html ../docs/source ../docs
Running Sphinx v4.5.0
making output directory... done
building [mo]: targets for 0 po files that are out of date
building [html]: targets for 6 source files that are out of date
updating environment: [new config] 6 added, 0 changed, 0 removed
reading sources... [100%] index
looking for now-outdated files... none found
pickling environment... done
checking consistency... done
preparing documents... done
writing output... [100%] index
generating indices... genindex done
writing additional pages... search done
copying images... [100%] images/programs.png
copying static files... done
copying extra files... done
dumping search index in English (code: en)... done
dumping object inventory... done
build succeeded.

KseniyaTikhomirova · 2023-06-14T13:37:25Z

@kbenzie Hi, I am trying to rebase changes to solve merge conflicts and script for spec generation silently fails now. It also fails on main branch without my changes. Is it something known? Could you advise how to solve it?

I just checkout out your branch and successfully built it locally, so I think its a local configuration hiccup you have.

My hunch is that your Python virtual environment was not enabled when make generate was invoked because I don't see any lines that look like this (from my local build):
Generating HTML...
sphinx-build -M html ../docs/source ../docs
Running Sphinx v4.5.0
making output directory... done
building [mo]: targets for 0 po files that are out of date
building [html]: targets for 6 source files that are out of date
updating environment: [new config] 6 added, 0 changed, 0 removed
reading sources... [100%] index
looking for now-outdated files... none found
pickling environment... done
checking consistency... done
preparing documents... done
writing output... [100%] index
generating indices... genindex done
writing additional pages... search done
copying images... [100%] images/programs.png
copying static files... done
copying extra files... done
dumping search index in English (code: en)... done
dumping object inventory... done
build succeeded.

I am able to build my branch locally too. The problem for me is with main. So I guess there is a change in configuration after code base I use for branch.

kbenzie · 2023-06-14T13:42:19Z

I am able to build my branch locally too. The problem for me is with main. So I guess there is a change in configuration after code base I use for branch.

Oh, I missed that. I have just checked out main and attempted a build locally which is also working as I'd expect so I'm not sure what's going on here.

Is your local main branch up to date with unified-runtime:main?

KseniyaTikhomirova · 2023-06-14T13:45:25Z

I am able to build my branch locally too. The problem for me is with main. So I guess there is a change in configuration after code base I use for branch.

Oh, I missed that. I have just checked out main and attempted a build locally which is also working as I'd expect so I'm not sure what's going on here.

Is your local main branch up to date with unified-runtime:main?

yes, I updated it today.

kbenzie · 2023-06-14T13:58:36Z

I am able to build my branch locally too. The problem for me is with main. So I guess there is a change in configuration after code base I use for branch.

Oh, I missed that. I have just checked out main and attempted a build locally which is also working as I'd expect so I'm not sure what's going on here.
Is your local main branch up to date with unified-runtime:main?

yes, I updated it today.

I'm at a bit of a loss then as I'm unable to reproduce this locally.

My last idea would be to try configuring a new build directory on the main branch to see if that works or not.

pbalcer · 2023-06-14T13:59:51Z

I've had a similar problem in the past. I was missing some sphinx dependency, and solved it with pip install -r third_party/requirements.txt.

We don't check in the generated docs, so running sphinx is not strictly necessary. It's just useful to make sure that the docs are still being built correctly. If you continue to have issues with your setup, you could also just disable this by adding --!html to the invocation of the run.py script here (https://github.com/oneapi-src/unified-runtime/blob/main/CMakeLists.txt#L236).
We could create an escape hatch like this in the CMake scripts themselves, but I'm not sure if we should :)

KseniyaTikhomirova · 2023-06-14T14:05:19Z

I've had a similar problem in the past. I was missing some sphinx dependency, and solved it with pip install -r third_party/requirements.txt.

We don't check in the generated docs, so running sphinx is not strictly necessary. It's just useful to make sure that the docs are still being built correctly. If you continue to have issues with your setup, you could also just disable this by adding --!html to the invocation of the run.py script here (https://github.com/oneapi-src/unified-runtime/blob/main/CMakeLists.txt#L236). We could create an escape hatch like this in the CMake scripts themselves, but I'm not sure if we should :)

will try it, thank you!

test/conformance/testing/include/uur/fixtures.h

KseniyaTikhomirova · 2023-06-15T09:13:35Z

I've had a similar problem in the past. I was missing some sphinx dependency, and solved it with pip install -r third_party/requirements.txt.

We don't check in the generated docs, so running sphinx is not strictly necessary. It's just useful to make sure that the docs are still being built correctly. If you continue to have issues with your setup, you could also just disable this by adding --!html to the invocation of the run.py script here (https://github.com/oneapi-src/unified-runtime/blob/main/CMakeLists.txt#L236). We could create an escape hatch like this in the CMake scripts themselves, but I'm not sure if we should :)

it works, thanks a lot!

KseniyaTikhomirova · 2023-06-15T09:14:31Z

tests are updated, rebased/merged with main. Ready to final review and merge.

aarongreig

LGTM

kbenzie

One last tweak to naming, I think this is ready to go after that change though.

scripts/core/kernel.yml

Signed-off-by: Tikhomirova, Kseniya <kseniya.tikhomirova@intel.com>

kbenzie · 2023-06-15T12:24:25Z

scripts/core/common.yml

      desc: $x_exp_command_buffer_desc_t
    - name: EXP_SAMPLER_MIP_PROPERTIES
      desc: $x_exp_sampler_mip_properties_t
+    - name: MEM_OBJ_PROPERTIES


to fix the build

Suggested change

- name: MEM_OBJ_PROPERTIES

- name: KERNEL_ARG_MEM_OBJ_PROPERTIES

fixed, thanks bee29ae

Signed-off-by: Tikhomirova, Kseniya <kseniya.tikhomirova@intel.com>

[UR] Proposal of new API for memory object properties

a5c6888

Signed-off-by: Tikhomirova, Kseniya <kseniya.tikhomirova@intel.com>

KseniyaTikhomirova mentioned this pull request Jun 6, 2023

SYCL RT request: need an API to provide memory access hint to urKernelSetArgMemObj #572

Closed

KseniyaTikhomirova marked this pull request as ready for review June 6, 2023 11:25

kbenzie suggested changes Jun 6, 2023

View reviewed changes

KseniyaTikhomirova added 5 commits June 6, 2023 07:17

Revert "[UR] Proposal of new API for memory object properties"

b63df59

This reverts commit a5c6888.

update scripts

cb72aad

Signed-off-by: Tikhomirova, Kseniya <kseniya.tikhomirova@intel.com>

update script, add structure type

4a252b1

Signed-off-by: Tikhomirova, Kseniya <kseniya.tikhomirova@intel.com>

add result of make generate

8e2c04b

Signed-off-by: Tikhomirova, Kseniya <kseniya.tikhomirova@intel.com>

fix properties type and update generated files

3186b53

Signed-off-by: Tikhomirova, Kseniya <kseniya.tikhomirova@intel.com>

KseniyaTikhomirova requested a review from kbenzie June 6, 2023 15:04

kbenzie reviewed Jun 6, 2023

View reviewed changes

scripts/core/kernel.yml Outdated Show resolved Hide resolved

KseniyaTikhomirova added 2 commits June 7, 2023 09:04

Merge branch 'main' into mem_obj_proposal

590ce6e

fix code-review comments

e6f5e81

Signed-off-by: Tikhomirova, Kseniya <kseniya.tikhomirova@intel.com>

KseniyaTikhomirova requested a review from kbenzie June 7, 2023 16:10

jandres742 reviewed Jun 7, 2023

View reviewed changes

kbenzie reviewed Jun 9, 2023

View reviewed changes

veselypeta suggested changes Jun 14, 2023

View reviewed changes

scripts/core/kernel.yml Outdated Show resolved Hide resolved

fix tests

8706202

Signed-off-by: Tikhomirova, Kseniya <kseniya.tikhomirova@intel.com>

kbenzie reviewed Jun 14, 2023

View reviewed changes

test/conformance/testing/include/uur/fixtures.h Outdated Show resolved Hide resolved

Merge branch 'main' into mem_obj_proposal

051b0c0

KseniyaTikhomirova requested review from aarongreig, jandres742, kbenzie and veselypeta June 15, 2023 09:13

aarongreig approved these changes Jun 15, 2023

View reviewed changes

kbenzie suggested changes Jun 15, 2023

View reviewed changes

scripts/core/kernel.yml Outdated Show resolved Hide resolved

scripts/core/kernel.yml Outdated Show resolved Hide resolved

kbenzie mentioned this pull request Jun 15, 2023

Add experimental USM Import/Release APIs. #614

Merged

rename structure

8539350

Signed-off-by: Tikhomirova, Kseniya <kseniya.tikhomirova@intel.com>

KseniyaTikhomirova requested a review from kbenzie June 15, 2023 12:21

kbenzie approved these changes Jun 15, 2023

View reviewed changes

veselypeta approved these changes Jun 15, 2023

View reviewed changes

fix name

bee29ae

Signed-off-by: Tikhomirova, Kseniya <kseniya.tikhomirova@intel.com>

kbenzie merged commit 7320835 into oneapi-src:main Jun 15, 2023

	- name: MEM_OBJ_PROPERTIES
	- name: KERNEL_ARG_MEM_OBJ_PROPERTIES

[UR] Proposal of new API for memory object properties #571

[UR] Proposal of new API for memory object properties #571

Uh oh!

Conversation

KseniyaTikhomirova commented Jun 6, 2023

Uh oh!

kbenzie left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KseniyaTikhomirova commented Jun 6, 2023

Uh oh!

kbenzie left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kbenzie commented Jun 7, 2023

Uh oh!

KseniyaTikhomirova commented Jun 7, 2023

Uh oh!

jandres742 Jun 7, 2023

Choose a reason for hiding this comment

Uh oh!

KseniyaTikhomirova Jun 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jandres742 Jun 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jandres742 Jun 14, 2023

Choose a reason for hiding this comment

Uh oh!

kbenzie Jun 14, 2023

Choose a reason for hiding this comment

Uh oh!

kbenzie commented Jun 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kbenzie left a comment

Choose a reason for hiding this comment

Uh oh!

KseniyaTikhomirova commented Jun 12, 2023

Uh oh!

smaslov-intel commented Jun 12, 2023

Uh oh!

KseniyaTikhomirova commented Jun 13, 2023

Uh oh!

jandres742 commented Jun 13, 2023

Uh oh!

smaslov-intel commented Jun 13, 2023

Uh oh!

veselypeta left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

KseniyaTikhomirova commented Jun 14, 2023

Uh oh!

kbenzie commented Jun 14, 2023

Uh oh!

KseniyaTikhomirova commented Jun 14, 2023

Uh oh!

kbenzie commented Jun 14, 2023

Uh oh!

KseniyaTikhomirova commented Jun 14, 2023

Uh oh!

kbenzie commented Jun 14, 2023

Uh oh!

pbalcer commented Jun 14, 2023

Uh oh!

KseniyaTikhomirova commented Jun 14, 2023

Uh oh!

Uh oh!

KseniyaTikhomirova commented Jun 15, 2023

Uh oh!

KseniyaTikhomirova commented Jun 15, 2023

Uh oh!

aarongreig left a comment

Choose a reason for hiding this comment

kbenzie left a comment •

edited

Loading

KseniyaTikhomirova Jun 9, 2023 •

edited

Loading

jandres742 Jun 9, 2023 •

edited

Loading

kbenzie commented Jun 9, 2023 •

edited

Loading