Skip to content
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

[SYCL] Remove experimental/builtins.hpp from sycl.hpp due to C++17 #6386

Merged
merged 3 commits into from Jul 1, 2022

Conversation

JackAKirk
Copy link
Contributor

@JackAKirk JackAKirk commented Jul 1, 2022

C++17 usage of if constexpr etc was added to experimental/builtins.hpp as requested in #5964, but I did not remove this header from sycl.hpp since there were no failing tests and I didn't notice it was included in sycl.hpp. Apparently sycl.hpp should not include any usage of C++17. This may be related to some of the failing tests that appear only on the CI: intel/llvm-test-suite#975 (comment). Necessary changes to the tests are added here : intel/llvm-test-suite#1072

Signed-off-by: JackAKirk jack.kirk@codeplay.com

Signed-off-by: JackAKirk <jack.kirk@codeplay.com>
@JackAKirk
Copy link
Contributor Author

/verify with intel/llvm-test-suite#1072

Signed-off-by: JackAKirk <jack.kirk@codeplay.com>
Signed-off-by: JackAKirk <jack.kirk@codeplay.com>
@JackAKirk
Copy link
Contributor Author

/verify with intel/llvm-test-suite#1072

@pvchupin pvchupin merged commit 2e1c36f into intel:sycl Jul 1, 2022
@bader
Copy link
Contributor

bader commented Jul 3, 2022

@pvchupin, please, do not merge pull requests introducing regressions. Pre-commit for this PR fails.

@JackAKirk, @pvchupin, I also think the PR must be reviewed before the merge.

@pvchupin
Copy link
Contributor

pvchupin commented Jul 5, 2022

@bader, I decided to took the risk here (on both merge and no-review) because this PR was fixing another set of issues appearing in many pre-commits that were hard to reproduce. I tested locally in the limited environment where reproducer actually succeeded, and testing passed but I didn't test all possible configurations and some new fails were unexpected.
In any case sorry about inconveniences it caused.
Thanks a lot for addressing remaining issues in intel/llvm-test-suite#1075.

Now we had a better sync on this let's proceed with solution in #6400.

bader pushed a commit that referenced this pull request Jul 11, 2022
Reverts #6386 so that experimental/builtins.hpp is included in sycl.hpp. This avoids users being required to include this header when using printf or experimental math functions.

Corresponding update here: intel/llvm-test-suite#1076.
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.

None yet

3 participants