Skip to content

[SYCL] Refactor include/sycl/properties/queue_properties.hpp #8048

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Jan 27, 2023
Merged

[SYCL] Refactor include/sycl/properties/queue_properties.hpp #8048

merged 3 commits into from
Jan 27, 2023

Conversation

aelovikov-intel
Copy link
Contributor

Make sure the required amount of boilerplate code is minimal. Fixes
an issue with missing explicit instantiations - see new symbols in the
ABI test(s).

@aelovikov-intel
[SYCL] Refactor include/sycl/properties/queue_properties.hpp
bdde265 
Make sure the required amount of boilerplate code is minimal. Fixes
an issue with missing explicit instantiations - see new symbols in the
ABI test(s).
@aelovikov-intel
Update Windows symbols
3b30a30
@aelovikov-intel aelovikov-intel requested a review from a team as a code owner January 18, 2023 23:16
@aelovikov-intel aelovikov-intel temporarily deployed to aws January 18, 2023 23:42 — with GitHub Actions Inactive
@aelovikov-intel aelovikov-intel temporarily deployed to aws January 19, 2023 00:45 — with GitHub Actions Inactive
@aelovikov-intel
Copy link
Contributor Author

CUDA's

SYCL :: BFloat16/bfloat16_builtins.cpp

is known - #8040.

@aelovikov-intel
Copy link
Contributor Author

@KseniyaTikhomirova , ping.

KseniyaTikhomirova
KseniyaTikhomirova reviewed Jan 23, 2023
View reviewed changes
Copy link
Contributor

@KseniyaTikhomirova KseniyaTikhomirova left a comment

Choose a reason for hiding this comment

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

To be honest I do not like that we introduce new macro. since we have many problems with user macro redefinitions I expected that we would prefer to avoid them.
Although if we still was to add them - should we follow this guide https://github.com/intel/llvm/blob/sycl/sycl/doc/developer/ContributeToDPCPP.md#general-guidelines-1? It says:
"All identifiers used in llvm/sycl headers files must contain at least one lowercase letter due to avoid conflicts with user-defined macros".

sycl/test/abi/sycl_symbols_linux.dump
@@ -3,7 +3,7 @@
# DO NOT EDIT IT MANUALLY. Refer to sycl/doc/developer/ABIPolicyGuide.md for more info.
################################################################################

# RUN: env LLVM_BIN_PATH=%llvm_build_bin_dir %python %sycl_tools_src_dir/abi_check.py --mode check_symbols --reference %s %sycl_libs_dir/libsycl.so
Copy link
Contributor

Choose a reason for hiding this comment

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

why you removed it? I guess it was done like this for systems where python3 installed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's what the script did.

sycl/test/abi/sycl_symbols_windows.dump
@@ -3,7 +3,7 @@
# DO NOT EDIT IT MANUALLY. Refer to sycl/doc/developer/ABIPolicyGuide.md for more info.
################################################################################

# RUN: env LLVM_BIN_PATH=%llvm_build_bin_dir %python %sycl_tools_src_dir/abi_check.py --mode check_symbols --reference %s %llvm_build_bin_dir/sycl6.dll
# RUN: env LLVM_BIN_PATH=%llvm_build_bin_dir python %sycl_tools_src_dir/abi_check.py --mode check_symbols --reference %s %llvm_build_bin_dir/sycl6.dll
Copy link
Contributor

Choose a reason for hiding this comment

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

same

@aelovikov-intel
Copy link
Contributor Author

To be honest I do not like that we introduce new macro. since we have many problems with user macro redefinitions I expected that we would prefer to avoid them. Although if we still was to add them - should we follow this guide https://github.com/intel/llvm/blob/sycl/sycl/doc/developer/ContributeToDPCPP.md#general-guidelines-1? It says: "All identifiers used in llvm/sycl headers files must contain at least one lowercase letter due to avoid conflicts with user-defined macros".

Modified new macros' names. Not changing their arguments because they don't seem to be affected (and we don't have the rule universally applied to macros arguments in our codebase either) - https://godbolt.org/z/hsn1o6dT8 .

@aelovikov-intel aelovikov-intel temporarily deployed to aws January 23, 2023 17:44 — with GitHub Actions Inactive
@aelovikov-intel aelovikov-intel temporarily deployed to aws January 23, 2023 18:15 — with GitHub Actions Inactive
@KseniyaTikhomirova KseniyaTikhomirova self-requested a review January 24, 2023 17:01
@KseniyaTikhomirova KseniyaTikhomirova temporarily deployed to aws January 24, 2023 17:57 — with GitHub Actions Inactive
@aelovikov-intel
Copy link
Contributor Author

Failure SYCL :: dword_atomic_smoke.cpp is known - #8098.

@aelovikov-intel
Copy link
Contributor Author

@intel/llvm-gatekeepers , PR is ready.

@steffenlarsen steffenlarsen merged commit 2864eac into intel:sycl Jan 27, 2023
@aelovikov-intel aelovikov-intel deleted the explicit-inst branch May 1, 2023 16:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KseniyaTikhomirova KseniyaTikhomirova KseniyaTikhomirova approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@aelovikov-intel @KseniyaTikhomirova @steffenlarsen