Skip to content

[SYCL][ABI-Break] Drop legacy overload of queue::mem_advice #14618

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

Conversation

AlexeySachkov
Copy link
Contributor

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.

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
Copy link
Contributor Author

Note for reviewers: this is one is a bit more radical, because it removes a user-facing API which we claimed to be present in the language. If we don't think that it is safe to remove it (note that #14145 also does an API-breaking change by changing pi_mem_advice argument to something from UR), then we should at least update the deprecation message saying that it is just deprecated and not deprecated in SYCL 2020 specifically.

@AlexeySachkov
Copy link
Contributor Author

FAIL: SYCL :: ESIMD/api/functional/ctors/ctor_load_usm_fp_extra.cpp (530 of 2106)
******************** TEST 'SYCL :: ESIMD/api/functional/ctors/ctor_load_usm_fp_extra.cpp' FAILED ********************
Exit Code: 1

Command Output (stdout):
--
# RUN: at line 8
D:/github/actions-runner/_work/llvm/llvm/install/bin/clang++.exe   -fsycl -fsycl-targets=spir[64](https://github.com/intel/llvm/actions/runs/9988053256/job/27606625538?pr=14618#step:12:65)  D:\github\actions-runner\_work\llvm\llvm\llvm\sycl\test-e2e\ESIMD\api\functional\ctors\ctor_load_usm_fp_extra.cpp -fsycl-device-code-split=per_kernel -o D:\github\actions-runner\_work\llvm\llvm\build-e2e\ESIMD\api\functional\ctors\Output\ctor_load_usm_fp_extra.cpp.tmp.out
# executed command: D:/github/actions-runner/_work/llvm/llvm/install/bin/clang++.exe -fsycl -fsycl-targets=spir64 'D:\github\actions-runner\_work\llvm\llvm\llvm\sycl\test-e2e\ESIMD\api\functional\ctors\ctor_load_usm_fp_extra.cpp' -fsycl-device-code-split=per_kernel -o 'D:\github\actions-runner\_work\llvm\llvm\build-e2e\ESIMD\api\functional\ctors\Output\ctor_load_usm_fp_extra.cpp.tmp.out'
# .---command stderr------------
# | D:\github\actions-runner\_work\llvm\llvm\install\bin\clang-offload-wrapper: error: 'C:\Users\GH_RUN~1\AppData\Local\Temp\lit-tmp-orqwpjll\ctor_load_usm_fp_extra-6b8723_esimd_14.sym': Operation did not complete successfully because the file contains a virus or potentially unwanted software.
# | clang++: error: clang-offload-wrapper command failed with exit code 1 (use -v to see invocation)
# `-----------------------------
# error: command failed with exit status: 1

--

I assume that this has nothing to do with my changes, but more of a sporadic hiccup of the OS

@jbrodman
Copy link
Contributor

The only form that was ever meant to exist was the one that took an int.

@AlexeySachkov AlexeySachkov merged commit 9b37c28 into intel:sycl Jul 18, 2024
14 of 15 checks passed
@AlexeySachkov AlexeySachkov deleted the private/asachkov/drop-legacy-mem-advice-method branch July 18, 2024 18:01
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.

3 participants