Skip to content

[SYCL] Enable builds on macOS host #6706

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 16 commits into from
Sep 27, 2022
Merged

[SYCL] Enable builds on macOS host #6706

merged 16 commits into from
Sep 27, 2022

Conversation

alexbatashev
Copy link
Contributor

@alexbatashev alexbatashev commented Sep 6, 2022

This patch contains minimal changes to make compiler toolchain builds succeed on macOS:

  • Add stubs for platform-dependent functions
  • Disable level zero plugin for macOS exclusively
  • Fix platform-dependent library names
  • Remove get_device_count_by_type tool (which is unused, and rather than fixing it, removal is much simpler)
  • Add minimal CI in post-commit to prevent build breakages in future

The above changes are far from fully working SYCL on Darwin targets, this patch only fixes toolchain builds, and does not attempt to make the toolchain itself functional.

Fixes #258
Fixes #982

@alexbatashev alexbatashev requested review from a team as code owners September 6, 2022 07:31
@alexbatashev
Copy link
Contributor Author

Test build logs for CI run should be available here soon: https://github.com/alexbatashev/llvm/runs/8201358091?check_suite_focus=true

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.

Code view for CI scripts.

bader
bader previously approved these changes Sep 6, 2022
@@ -31,7 +31,6 @@ list(APPEND SYCL_TEST_DEPS
sycl-toolchain
FileCheck
not
get_device_count_by_type
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it dead? I thought we use it in either in-tree LIT or llvm-test-suite tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought so, too. Turns out, there's not a single mention of this tool, other than these, that I'd removed. It was dead for a long time now. LLVM Test Suite does not check for devices at all, fully relying on user input, and device-dependent tests have been removed from this repo.

@alexbatashev alexbatashev added the disable-lint Skip linter check step and proceed with build jobs label Sep 7, 2022
@alexbatashev
Copy link
Contributor Author

No idea why clang-format is triggered by a file, that is not changed in this PR

@alexbatashev alexbatashev reopened this Sep 7, 2022
@bader
Copy link
Contributor

bader commented Sep 7, 2022

No idea why clang-format is triggered by a file, that is not changed in this PR

I suppose it's due to this change: #6652. There is a new way to generate the diff, which I suspect might be the reason of this.

@alexbatashev alexbatashev requested a review from bader September 8, 2022 06:39
@alexbatashev
Copy link
Contributor Author

@intel/llvm-reviewers-runtime friendly ping

@bader bader removed the disable-lint Skip linter check step and proceed with build jobs label Sep 8, 2022
@alexbatashev
Copy link
Contributor Author

Both fails seem to be true for other PRs/commits. Tracker for hung config: #6732

#define __SYCL_OPENCL_PLUGIN_NAME "libpi_opencl.so"
#define __SYCL_LEVEL_ZERO_PLUGIN_NAME "libpi_level_zero.so"
#define __SYCL_CUDA_PLUGIN_NAME "libpi_cuda.so"
#define __SYCL_ESIMD_EMULATOR_PLUGIN_NAME "libpi_esimd_emulator.so"
#define __SYCL_HIP_PLUGIN_NAME "libpi_hip.so"
#elif defined(__SYCL_RT_OS_DARWIN)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I wonder if we simply need to define a suffix (like .dll, .so or .dylib) to use it in library names instead of duplicating almost the same lines three times

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The right way to do this is to autogenerate these names from CMake, but that'd be outside of the scope of this patch.

@bader
Copy link
Contributor

bader commented Sep 22, 2022

@alexbatashev, could you resolve merge conflicts, please?

@bader
Copy link
Contributor

bader commented Sep 27, 2022

@alexbatashev, probably another merge is required. Could you take look at pre-commit failures, please?

@bader bader merged commit 60c634c into intel:sycl Sep 27, 2022
sergey-semenov added a commit that referenced this pull request Sep 6, 2023
This tool is not needed, we stopped building it a while back in
#6706
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.

[MacOS] Minimal support/CI pipeline for MacOS ? [MacOS] directory copy of OpenCL fails
4 participants