[CI] Use oneAPI for Windows postcommit builds #16355

sarnex · 2024-12-12T22:57:41Z

Getting this working was an unbelievable amount of work. I hit so many weird issues caused by dumb Windows things (case insensitivity, cmd vs bash, bugs in cmake/ninja/sccache themselves) but finally I got something that consistently passes.

You can see the funniest dumb Windows issue in the comment in the CMake file change.

The basic idea is to extend the Windows build workflow to allow using icx, add an action to setup the oneAPI environment, call it from the build action and the test action (since we need some of the shared libs on path even for testing), and fix some oneAPI-only build issues.

We need to do some Powershell magic after calling the oneAPI setup bat script because the environment variables get lost since it's run in a subprocess.

We also switch to ccache instead of sccache, because sccache doesn't support icx (actually neither does ccache, but I added it upstream here and we are using a local build of ccache, it was easier to add support to ccache). Manually tested it to confirmed cache reads and write are working as expected.

At some point we should probably investigate why some tests fail or some compiler flags are required with icx only and not cl, but let's do that as separate work because I am done with Windows for now.

I already set up all Windows runners used here to work with this change.

Next I'll add oneAPI builds on Linux, which I really hope is easier.

Signed-off-by: Sarnie, Nick <nick.sarnie@intel.com>

aelovikov-intel · 2024-12-13T16:19:55Z

We also switch to ccache instead of sccache

@bader , do you know the history why we used sccache on win?

sarnex · 2024-12-13T16:23:41Z

Google says they are exactly the same except sccache supports Rust, which we shouldn't care about. Also I have no idea how to add icx support to sccache, it was trivial for ccache.

uditagarwal97 · 2024-12-13T16:31:08Z

.github/workflows/sycl-windows-build.yml

      run: |
-        cmake --build build --target check-sycl
+         if [[ ${{inputs.compiler}} == 'icx' ]]; then
+           export LIT_FILTER_OUT="host_tanpi_double_accuracy"


Was host_tanpi_double_accuracy failing with icx? Do we have any related bug report?

yeah this and some LIT tests are failing when the compiler is built with icx. I don't know why, I made #16362 to look at it later, I've had enough with windows for a few days

aelovikov-intel · 2024-12-13T16:21:24Z

.github/workflows/sycl-post-commit.yml

-
+    with:
+      compiler: icx
+      build_configure_extra_args: --cmake-opt=-DCMAKE_C_FLAGS="/clang:-Wno-nonportable-include-path /clang:-Wno-cast-function-type-mismatch" --cmake-opt=-DCMAKE_CXX_FLAGS="/clang:-Wno-nonportable-include-path /clang:-Wno-cast-function-type-mismatch" --cmake-opt="-DCMAKE_EXE_LINKER_FLAGS=/manifest:no" --cmake-opt="-DCMAKE_MODULE_LINKER_FLAGS=/manifest:no" --cmake-opt="-DCMAKE_SHARED_LINKER_FLAGS=/manifest:no" 


I think this should go to CMake instead, but it's been a tremendous amount of work already, so I won't insist :)

agree but yeah please allow me some time regain my sanity, will do in follow up

made #16363 for follow up

aelovikov-intel · 2024-12-13T16:23:06Z

.github/workflows/sycl-windows-build.yml

+        type: choice
+        options:
+          - cl
+          - icx


Will it work without build_configure_extra_args for manual dispatch? Or am I missing something?

we need this to pass the compiler flags we set in sycl-post-commit.yml, and then we use it an an envvar named ARGS in this file that is passed to configure.py, we need the compiler flags to get the thing to build

talked offline, fixed in latest commit, thx

aelovikov-intel · 2024-12-13T16:24:37Z

.github/workflows/sycl-windows-build.yml

      run: |
+        if [[ ${{inputs.compiler}} == 'icx' ]]; then


Similarly to CMake options, that's better be handled elsewhere (LIT feature + REQUIRES?)

yeah i think its reasonable to add a LIT feature but hopefully I can do this in a follow-up commit

made #16363 for follow up

aelovikov-intel · 2024-12-13T16:26:03Z

sycl/source/CMakeLists.txt

+  # When building using icx on Windows, the VERSION file
+  # produced by cmake is used in source code
+  # when including '<version>' because Windows is
+  # case-insensitive and icx adds the build directory
+  # to the system header search path.


Wow... Just speechless...

Do you know if "VERSION" is some standard naming? If not, maybe we can rename it instead of this workaround.

the variable as a CMake variable is standard but writing it to disk seems to be somewhat standard but not done by every project. im pretty sure here it's coming from this code.

aelovikov-intel · 2024-12-13T16:31:34Z

sycl/unittests/assert/assert.cpp

 static constexpr int MemoryMapCounterBase = 1000;
 static int MemoryMapCounter = MemoryMapCounterBase;
+#ifndef _WIN32


For other reviewers - looks like we have some tests excluded on _WIN32 making these static functions unused. I suppose icx emits a warning about that and then it gets turned into a error due to -Werror.

yeah tbh i dont know why these dont error with cl, but they seem unused always on windows so this change should be safe for any compiler

bader · 2024-12-13T17:07:15Z

We also switch to ccache instead of sccache

@bader , do you know the history why we used sccache on win?

I don't remember all the details. From what I recall, we tried ccache and it didn't work for us back then. Either there was some issues with using it with MSVC or some issues with installing ccache in CI.
IIRC, sccache has some unique features missing in ccache like being able to store cache on remote machine, but I don't think we utilize them in our workflow.

Signed-off-by: Sarnie, Nick <nick.sarnie@intel.com>

aelovikov-intel

Thanks for all the heavy uplift done here. Future improvements/cleanup can be done in other PRs (possibly by other folks :)).

aarongreig

UR LGTM

bader · 2024-12-13T17:23:10Z

We also switch to ccache instead of sccache, because sccache doesn't support icx (actually neither does ccache, but I added it upstream here and we are using a local build of ccache, it was easier to add support to ccache). Manually tested it to confirmed cache reads and write are working as expected.

I see that you added support only for MSVC compatible drivers: icx and icx-cl. Intel Compiler provides Clang-compatible icpx and icx-cc on Windows as well. Shouldn't ccache support these drivers as well?

sarnex · 2024-12-13T17:26:21Z

@bader Yeah probably but I'd prefer to do it in another PR after upstream merges this one, I'll retitle it to be clear it's MSVC only

[CI] Use oneAPI for Windows postcommit builds

130530e

Signed-off-by: Sarnie, Nick <nick.sarnie@intel.com>

sarnex temporarily deployed to WindowsCILock December 12, 2024 22:58 — with GitHub Actions Inactive

sarnex temporarily deployed to WindowsCILock December 12, 2024 22:59 — with GitHub Actions Inactive

sarnex temporarily deployed to WindowsCILock December 12, 2024 23:55 — with GitHub Actions Inactive

sarnex temporarily deployed to WindowsCILock December 13, 2024 00:45 — with GitHub Actions Inactive

sarnex marked this pull request as ready for review December 13, 2024 16:12

sarnex requested review from a team as code owners December 13, 2024 16:12

sarnex requested a review from aelovikov-intel December 13, 2024 16:12

uditagarwal97 reviewed Dec 13, 2024

View reviewed changes

aelovikov-intel reviewed Dec 13, 2024

View reviewed changes

sarnex mentioned this pull request Dec 13, 2024

Investigate LIT and E2E failures when compiler is build with icx on WIndows #16362

Open

fix manual workflow

3ec9cbd

Signed-off-by: Sarnie, Nick <nick.sarnie@intel.com>

sarnex temporarily deployed to WindowsCILock December 13, 2024 17:08 — with GitHub Actions Inactive

sarnex mentioned this pull request Dec 13, 2024

Improve icx Windows workarounds #16363

Open

aelovikov-intel approved these changes Dec 13, 2024

View reviewed changes

aarongreig approved these changes Dec 13, 2024

View reviewed changes

sarnex temporarily deployed to WindowsCILock December 13, 2024 17:43 — with GitHub Actions Inactive

sarnex temporarily deployed to WindowsCILock December 13, 2024 18:30 — with GitHub Actions Inactive

sarnex requested a review from uditagarwal97 December 13, 2024 19:01

uditagarwal97 approved these changes Dec 13, 2024

View reviewed changes

sarnex merged commit 4e8b647 into intel:sycl Dec 13, 2024
23 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CI] Use oneAPI for Windows postcommit builds #16355

[CI] Use oneAPI for Windows postcommit builds #16355

sarnex commented Dec 12, 2024 •

edited

Loading

aelovikov-intel commented Dec 13, 2024

sarnex commented Dec 13, 2024 •

edited

Loading

uditagarwal97 Dec 13, 2024

sarnex Dec 13, 2024

aelovikov-intel Dec 13, 2024

sarnex Dec 13, 2024

sarnex Dec 13, 2024

aelovikov-intel Dec 13, 2024

sarnex Dec 13, 2024

sarnex Dec 13, 2024

aelovikov-intel Dec 13, 2024

sarnex Dec 13, 2024

sarnex Dec 13, 2024

aelovikov-intel Dec 13, 2024

sarnex Dec 13, 2024 •

edited

Loading

aelovikov-intel Dec 13, 2024

sarnex Dec 13, 2024 •

edited

Loading

bader commented Dec 13, 2024

aelovikov-intel left a comment

aarongreig left a comment

bader commented Dec 13, 2024

sarnex commented Dec 13, 2024

[CI] Use oneAPI for Windows postcommit builds #16355

[CI] Use oneAPI for Windows postcommit builds #16355

Conversation

sarnex commented Dec 12, 2024 • edited Loading

aelovikov-intel commented Dec 13, 2024

sarnex commented Dec 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sarnex Dec 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sarnex Dec 13, 2024 • edited Loading

Choose a reason for hiding this comment

bader commented Dec 13, 2024

aelovikov-intel left a comment

Choose a reason for hiding this comment

aarongreig left a comment

Choose a reason for hiding this comment

bader commented Dec 13, 2024

sarnex commented Dec 13, 2024

sarnex commented Dec 12, 2024 •

edited

Loading

sarnex commented Dec 13, 2024 •

edited

Loading

sarnex Dec 13, 2024 •

edited

Loading

sarnex Dec 13, 2024 •

edited

Loading