Skip to content

[Exp] Add Import/Release USM APIs using .yml.#584

Closed
rdeodhar wants to merge 4 commits intooneapi-src:mainfrom
rdeodhar:ur_l0import
Closed

[Exp] Add Import/Release USM APIs using .yml.#584
rdeodhar wants to merge 4 commits intooneapi-src:mainfrom
rdeodhar:ur_l0import

Conversation

@rdeodhar
Copy link
Contributor

@rdeodhar rdeodhar commented Jun 8, 2023

This change adds the definitions necessary for two new functions: USMImport and USMRelease.

USMImport converts host system memory into something akin to USM host memory.
USMRelease undoes the effect of a previous USMImport.

This functionality is used by SYCL to import/release host memory to speed up host <-> device data transfers.

@kbenzie kbenzie added the experimental Experimental feature additions/changes/specification label Jun 9, 2023
@rdeodhar rdeodhar requested a review from kbenzie June 9, 2023 20:30
@kbenzie kbenzie changed the title Add Import/Release USM APIs using .yml. [Exp] Add Import/Release USM APIs using .yml. Jun 12, 2023
Copy link
Contributor

@veselypeta veselypeta left a comment

Choose a reason for hiding this comment

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

Copy link
Contributor

@kbenzie kbenzie left a comment

Choose a reason for hiding this comment

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

API changes look good, I echo @veselypeta's feedback about adding a .rst document describing the feature.

@rdeodhar
Copy link
Contributor Author

I've added the file EXP-USM-IMPORT-RELEASE.rst as documentation.
How to use the $x notation was not clear to me. Please check.

@rdeodhar rdeodhar requested a review from veselypeta June 13, 2023 20:47
Copy link
Contributor

@veselypeta veselypeta left a comment

Choose a reason for hiding this comment

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

LGTM. Just a suggestion on when you need to rebase your changes.

Comment on lines 367 to 372
- name: USM_IMPORT_EXP
desc: Enumerator for $xUSMImportExp
value: '122'
- name: USM_RELEASE_EXP
desc: Enumerator for $xUSMReleaseExp
value: '123'
Copy link
Contributor

Choose a reason for hiding this comment

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

When you rebase - I recommend deleting these additions. After you rebase you can then rerun the generate script and it will assign you new values for these entry points. This is an unfortunate consequence of how the registry is generated.

.. parsed-literal::

// Import into USM
USMImport(hContext, hostPtr, size);
Copy link
Contributor

Choose a reason for hiding this comment

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

In rst files use ${x}/${X} for the prefix:

Suggested change
USMImport(hContext, hostPtr, size);
${x}USMImport(hContext, hostPtr, size);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I prefix the function name with "${x}" then the generate step emits this error message:
Generating ../docs/source/core/EXP-USM-IMPORT-RELEASE.rst...
/iusers/rdeodhar/cmplr/ur_l0import/llvm/scripts/core/EXP-USM-IMPORT-RELEASE.rst(45) : error : symbol '$xUSMImport' not found
/iusers/rdeodhar/cmplr/ur_l0import/llvm/scripts/core/EXP-USM-IMPORT-RELEASE.rst(57) : error : symbol '$xUSMRelease' not found
Failed to generate specification.

That's why I had left it out. Perhaps x needs to be defined somewhere. Not clear to me how/where.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another question about the generated files:
Are some of them read-modify-write, or are they all simply generated, i.e. written?

Copy link
Contributor

Choose a reason for hiding this comment

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

That's why I had left it out. Perhaps x needs to be defined somewhere. Not clear to me how/where.

Okay, I'll have a look at this and see if the generator script is doing something different for this file compared to the others.

Are some of them read-modify-write, or are they all simply generated, i.e. written?

So anything in the include and source directories which the generator scripts touch are simply generated. scripts/core/registry.yml is different and it read/modify/write but only when a function is added/updated/removed.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's why I had left it out. Perhaps x needs to be defined somewhere. Not clear to me how/where.

Okay, I'll have a look at this and see if the generator script is doing something different for this file compared to the others.

Are some of them read-modify-write, or are they all simply generated, i.e. written?

So anything in the include and source directories which the generator scripts touch are simply generated. scripts/core/registry.yml is different and it read/modify/write but only when a function is added/updated/removed.

The generator script validation was actually correct, these are missing the Exp suffix. I'll make suggestions on your new PR.

.. parsed-literal::

// Release from USM
USMRelease(hContext, hostPtr);
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
USMRelease(hContext, hostPtr);
${x}USMRelease(hContext, hostPtr);

@rdeodhar rdeodhar closed this Jun 14, 2023
@rdeodhar
Copy link
Contributor Author

rdeodhar commented Jun 14, 2023

After doing a merge there were many conflicts.
Restarting in a new branch.
PR is #614

@kbenzie
Copy link
Contributor

kbenzie commented Jun 15, 2023

Restarting in a new branch.

If we can avoid this in future that would be very much appriciated. It makes reviewing and keeping track of changes much more difficult than it needs to be.

@kbenzie
Copy link
Contributor

kbenzie commented Jun 15, 2023

After doing a merge there were many conflicts.

We do provide guidance on resolving conflicts in the hint box of the forks and pullrequests section of the contribution guide which may have helped.

@rdeodhar
Copy link
Contributor Author

Moving to a new PR was a desperate measure. Sorry about that. The number and nature of conflicts seemed unreasonable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

experimental Experimental feature additions/changes/specification

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants