Skip to content

[SYCL] Replaces some of the CL_* enums with PI_* enums. #1239

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

Merged
merged 4 commits into from
Mar 12, 2020

Conversation

rbegam
Copy link
Contributor

@rbegam rbegam commented Mar 3, 2020

Signed-off-by: rbegam [email protected]

Co-Authored-By: Alexey Bader [email protected]

@bader
Copy link
Contributor

bader commented Mar 5, 2020

@rbegam, please, resolve merge conflict and clang-format check issues.

@rbegam rbegam force-pushed the private/rbegam/sycl-rename-new branch 2 times, most recently from a9ef255 to 5a14600 Compare March 6, 2020 21:15
@bader
Copy link
Contributor

bader commented Mar 10, 2020

@rbegam, please, resolve merge conflicts.

@rbegam rbegam force-pushed the private/rbegam/sycl-rename-new branch from 5a14600 to 107c86d Compare March 10, 2020 17:43
@rbegam
Copy link
Contributor Author

rbegam commented Mar 10, 2020

@bader done. please review.

Copy link
Contributor

@bader bader left a comment

Choose a reason for hiding this comment

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

Overall LGTM, just a few nits.

@rbegam
Copy link
Contributor Author

rbegam commented Mar 12, 2020

@romanovvlad all comments have been resolved. please submit as I do not have write permission. thanks.

@bader bader merged commit 092367b into intel:sycl Mar 12, 2020
@rbegam rbegam deleted the private/rbegam/sycl-rename-new branch March 12, 2020 20:00
PI_COMMAND_EVENTS_WAIT = CL_COMMAND_MARKER,
PI_COMMAND_MEMBUFFER_COPY = CL_COMMAND_COPY_BUFFER,
PI_COMMAND_MEMBUFFER_FILL = CL_COMMAND_FILL_BUFFER
} _pi_command_type;
Copy link
Contributor

Choose a reason for hiding this comment

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

These changes break the CUDA plugin, e.g.

_pi_event::make_native(PI_COMMAND_MEMBUFFER_WRITE, command_queue));
et al.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Those are renamed i.e., PI_COMMAND_MEMBUFFER_WRITE -> PI_COMMAND_TYPE_MEM_BUFFER_WRITE

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, but the renaming was not propagated to the CUDA plugin.

Copy link
Contributor

Choose a reason for hiding this comment

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

@fwyzard, #1310 should fix that issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, that worked.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fwyzard sorry about that. Why wasn't there any indication in the tests that it broke CUDA plugin? Was I needed to run some other tests by myself? @bader

Copy link
Contributor

Choose a reason for hiding this comment

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

@fwyzard sorry about that. Why wasn't there any indication in the tests that it broke CUDA plugin? Was I needed to run some other tests by myself?

+@pvchupin to reply

Copy link
Contributor

Choose a reason for hiding this comment

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

There is no buildbot to check that at the moment, but we are working on adding it.
Meanwhile to check locally you can try this https://github.com/intel/llvm/blob/sycl/sycl/doc/GetStartedGuide.md#build-dpc-toolchain-with-support-for-nvidia-cuda

bader pushed a commit that referenced this pull request Mar 14, 2020
The CUDA backend cannot currently build due to the renaming of descriptors done in #1239. This PR adjusts the backend to the new descriptor names.

Signed-off-by: Steffen Larsen <[email protected]>
AlexeySachkov added a commit to AlexeySachkov/llvm that referenced this pull request Jul 18, 2024
We made a mistake several years ago by introducing `pi_mem_advice`
argument into a public interface in intel#1239.

This mistake was caught a year later and addressed in intel#4100,
but unfortunately instead of removing this incorrect overload, it was
deprecated. Even deprectation was done improperly, because this method
is *not* deprecated in SYCL 2020 - it *doesn't exists* in SYCL 2020.

This PR removes the method completely, even though our messaging was
wrong. The assumption is that there are no actual users who went and
used undocumented non-public `pi_mem_advice` in their codebases.
AlexeySachkov added a commit that referenced this pull request Jul 18, 2024
We made a mistake several years ago by introducing `pi_mem_advice`
argument into a public interface in #1239.

This mistake was caught a year later and addressed in #4100,
but unfortunately instead of removing this incorrect overload, it was
only deprecated. The deprecation wasn't done quite correctly, because
this method is *not* deprecated in SYCL 2020 - it *doesn't exists* in
SYCL 2020.

This PR removes the method completely, even though our messaging was
wrong. The assumption is that there are no actual users who went and
used undocumented non-public `pi_mem_advice` in their codebases.

This is patch is essentially a by-product of #14145 and it is
done to simplify that change, i.e. PI plugins removal should not be
ABI-breaking by itself, we just need to cleanup some of our exported
symbols.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants