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

[CI] Use oneAPI for Windows postcommit builds #16355

Merged
merged 2 commits into from
Dec 13, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion .github/workflows/sycl-post-commit.yml
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,11 @@ jobs:
&& success()
&& github.repository == 'intel/llvm'
uses: ./.github/workflows/sycl-windows-build.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"
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

made #16363 for follow up

build_cache_suffix: icx

e2e-win:
needs: build-win
# Continue if build was successful.
Expand All @@ -117,6 +121,7 @@ jobs:
runner: '["Windows","gen12"]'
sycl_toolchain_archive: ${{ needs.build-win.outputs.artifact_archive_name }}
extra_lit_opts: --param gpu-intel-gen12=True
compiler: icx

macos_default:
name: macOS
Expand Down
51 changes: 39 additions & 12 deletions .github/workflows/sycl-windows-build.yml
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,9 @@ on:
build_ref:
type: string
required: false
build_configure_extra_args:
type: string
required: false
changes:
type: string
description: 'Filter matches for the changed files in the PR'
Expand All @@ -22,6 +25,10 @@ on:
description: 'Artifacts retention period'
type: string
default: 3
compiler:
type: string
required: false
default: "cl"

outputs:
build_conclusion:
Expand Down Expand Up @@ -50,6 +57,12 @@ on:
type: choice
options:
- 3
compiler:
type: choice
options:
- cl
- icx
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

talked offline, fixed in latest commit, thx


permissions: read-all

jobs:
Expand All @@ -61,37 +74,43 @@ jobs:
outputs:
build_conclusion: ${{ steps.build.conclusion }}
steps:
- uses: actions/checkout@v4
with:
path: src
ref: ${{ inputs.build_ref || github.sha }}
fetch-depth: 1
- uses: ilammy/msvc-dev-cmd@0b201ec74fa43914dc39ae48a89fd1d8cb592756
with:
arch: amd64
- name: Setup oneAPI env
uses: ./src/devops/actions/setup_windows_oneapi_env
if: ${{ always() && !cancelled() && inputs.compiler == 'icx' }}
- name: Set env
run: |
git config --system core.longpaths true
git config --global core.autocrlf false
echo "C:\Program Files\Git\usr\bin" | Out-File -FilePath $env:GITHUB_PATH -Encoding utf8 -Append
echo "SCCACHE_DIR=D:\github\_work\cache\${{ inputs.build_cache_suffix }}" | Out-File -FilePath $env:GITHUB_ENV -Encoding utf8 -Append
- uses: actions/checkout@v4
with:
path: src
ref: ${{ inputs.build_ref || github.sha }}
fetch-depth: 1
echo "CCACHE_DIR=D:\github\_work\cache\${{ inputs.build_cache_suffix }}" | Out-File -FilePath $env:GITHUB_ENV -Encoding utf8 -Append
echo "CCACHE_MAXSIZE=10G" | Out-File -FilePath $env:GITHUB_ENV -Encoding utf8 -Append
- name: Register cleanup after job is finished
uses: ./src/devops/actions/cleanup
- name: Configure
shell: cmd
env:
ARGS: ${{ inputs.build_configure_extra_args }}
# TODO switch to clang-cl and lld when this is fixed https://github.com/oneapi-src/level-zero/issues/83
run: |
mkdir build
mkdir install
IF NOT EXIST D:\github\_work\cache MKDIR D:\github\_work\cache
IF NOT EXIST D:\github\_work\cache\${{inputs.build_cache_suffix}} MKDIR D:\github\_work\cache\${{inputs.build_cache_suffix}}
python.exe src/buildbot/configure.py -o build ^
--ci-defaults ^
--cmake-opt="-DCMAKE_C_COMPILER=cl" ^
--cmake-opt="-DCMAKE_CXX_COMPILER=cl" ^
--ci-defaults %ARGS% ^
--cmake-opt="-DCMAKE_C_COMPILER=${{inputs.compiler}}" ^
--cmake-opt="-DCMAKE_CXX_COMPILER=${{inputs.compiler}}" ^
--cmake-opt="-DCMAKE_INSTALL_PREFIX=%GITHUB_WORKSPACE%\install" ^
--cmake-opt="-DCMAKE_CXX_COMPILER_LAUNCHER=sccache" ^
--cmake-opt="-DCMAKE_C_COMPILER_LAUNCHER=sccache" ^
--cmake-opt="-DCMAKE_CXX_COMPILER_LAUNCHER=ccache" ^
--cmake-opt="-DCMAKE_C_COMPILER_LAUNCHER=ccache" ^
--cmake-opt="-DLLVM_INSTALL_UTILS=ON" ^
--cmake-opt="-DLLVM_EXPERIMENTAL_TARGETS_TO_BUILD=SPIRV"
- name: Build
Expand All @@ -101,16 +120,24 @@ jobs:
cmake --build build --target sycl-toolchain
- name: check-llvm
if: always() && !cancelled() && contains(inputs.changes, 'llvm')
shell: bash
run: |
if [[ ${{inputs.compiler}} == 'icx' ]]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

made #16363 for follow up

export LIT_FILTER="SYCL"
fi
cmake --build build --target check-llvm
- name: check-clang
if: always() && !cancelled() && contains(inputs.changes, 'clang')
run: |
cmake --build build --target check-clang
- name: check-sycl
if: always() && !cancelled() && contains(inputs.changes, 'sycl')
shell: bash
run: |
cmake --build build --target check-sycl
if [[ ${{inputs.compiler}} == 'icx' ]]; then
export LIT_FILTER_OUT="host_tanpi_double_accuracy"
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

fi
cmake --build build --target check-sycl
- name: check-sycl-unittests
if: always() && !cancelled() && contains(inputs.changes, 'sycl')
run: |
Expand Down
23 changes: 17 additions & 6 deletions .github/workflows/sycl-windows-run-tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,11 @@ on:
default: '{}'
required: False

compiler:
type: string
required: false
default: "cl"

permissions: read-all

jobs:
Expand All @@ -42,20 +47,23 @@ jobs:
environment: WindowsCILock
env: ${{ fromJSON(inputs.env) }}
steps:
# TODO: use cached_checkout
- uses: actions/checkout@v4
with:
persist-credentials: false
ref: ${{ inputs.ref || github.sha }}
path: llvm
- uses: ilammy/msvc-dev-cmd@0b201ec74fa43914dc39ae48a89fd1d8cb592756
with:
arch: amd64
- name: Setup oneAPI env
uses: ./llvm/devops/actions/setup_windows_oneapi_env
if: ${{ always() && !cancelled() && inputs.compiler == 'icx' }}
- name: Set env
run: |
git config --system core.longpaths true
git config --global core.autocrlf false
echo "C:\Program Files\Git\usr\bin" | Out-File -FilePath $env:GITHUB_PATH -Encoding utf8 -Append
# TODO: use cached_checkout
- uses: actions/checkout@v4
with:
persist-credentials: false
ref: ${{ inputs.ref || github.sha }}
path: llvm
- name: Register cleanup after job is finished
uses: ./llvm/devops/actions/cleanup
- name: Download compiler toolchain
Expand Down Expand Up @@ -84,6 +92,9 @@ jobs:
shell: bash
run: |
# Run E2E tests.
if [[ ${{inputs.compiler}} == 'icx' ]]; then
export LIT_FILTER_OUT="compile_on_win_with_mdd"
fi
export LIT_OPTS="-v --no-progress-bar --show-unsupported --show-pass --show-xfail --max-time 3600 --time-tests ${{ inputs.extra_lit_opts }}"
cmake --build build-e2e --target check-sycl-e2e
- name: Detect hung tests
Expand Down
28 changes: 28 additions & 0 deletions devops/actions/setup_windows_oneapi_env/action.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
name: Windows setup oneAPI env

runs:
using: "composite"
steps:
- name: Setup oneAPI env
shell: powershell
run: |
$batchFilePath = "C:\Program Files (x86)\Intel\oneAPI\setvars.bat"

$githubEnvFilePath = $env:GITHUB_ENV

$envBefore = Get-ChildItem Env: | ForEach-Object { "$($_.Name)=$($_.Value)" }

$envVars = & cmd.exe /c "call `"$batchFilePath`" && set" | Out-String

$envAfter = $envVars -split "`r`n" | Where-Object { $_ -match "^(.*?)=(.*)$" }

foreach ($envVar in $envAfter) {
if ($envVar -match "^(.*?)=(.*)$") {
$name = $matches[1]
$value = $matches[2]
$envBeforeVar = $envBefore | Where-Object { $_ -like "$name=*" }
if (-not $envBeforeVar -or $envBeforeVar -ne "$name=$value") {
Add-Content -Path $githubEnvFilePath -Value "$name=$value"
}
}
}
12 changes: 12 additions & 0 deletions sycl/source/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -222,6 +222,18 @@ function(add_sycl_rt_library LIB_NAME LIB_OBJ_NAME)
target_compile_options(${LIB_OBJ_NAME} PRIVATE
-Winstantiation-after-specialization)
endif()

# 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.
Comment on lines +226 to +230
Copy link
Contributor

Choose a reason for hiding this comment

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

Wow... Just speechless...

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

Copy link
Contributor Author

@sarnex sarnex Dec 13, 2024

Choose a reason for hiding this comment

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

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.

if (WIN32 AND CMAKE_CXX_COMPILER_ID MATCHES "IntelLLVM")
set(VERSION_FILE "${CMAKE_BINARY_DIR}/VERSION")
if(EXISTS ${VERSION_FILE})
file(REMOVE ${VERSION_FILE})
endif()
endif()
endfunction(add_sycl_rt_library)

set(SYCL_COMMON_SOURCES
Expand Down
18 changes: 0 additions & 18 deletions sycl/source/detail/windows_ur.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -56,24 +56,6 @@ void *getOsLibraryFuncAddress(void *Library, const std::string &FunctionName) {
GetProcAddress((HMODULE)Library, FunctionName.c_str()));
}

static std::filesystem::path getCurrentDSODirPath() {
wchar_t Path[MAX_PATH];
auto Handle =
getOSModuleHandle(reinterpret_cast<void *>(&getCurrentDSODirPath));
DWORD Ret = GetModuleFileName(
reinterpret_cast<HMODULE>(ExeModuleHandle == Handle ? 0 : Handle), Path,
MAX_PATH);
assert(Ret < MAX_PATH && "Path is longer than MAX_PATH?");
assert(Ret > 0 && "GetModuleFileName failed");
(void)Ret;

BOOL RetCode = PathRemoveFileSpec(Path);
assert(RetCode && "PathRemoveFileSpec failed");
(void)RetCode;

return std::filesystem::path(Path);
}

void *getURLoaderLibrary() { return getPreloadedURLib(); }

} // namespace ur
Expand Down
8 changes: 7 additions & 1 deletion sycl/unittests/assert/assert.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -145,10 +145,12 @@ static AssertHappened ExpectedToOutput = {
};

static constexpr int KernelLaunchCounterBase = 0;
static int KernelLaunchCounter = KernelLaunchCounterBase;
static constexpr int MemoryMapCounterBase = 1000;
static int MemoryMapCounter = MemoryMapCounterBase;
#ifndef _WIN32
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@sarnex sarnex Dec 13, 2024

Choose a reason for hiding this comment

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

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

static int KernelLaunchCounter = KernelLaunchCounterBase;
static constexpr int PauseWaitOnIdx = KernelLaunchCounterBase + 1;
#endif

// Mock redifinitions
static ur_result_t redefinedKernelGetGroupInfoAfter(void *pParams) {
Expand All @@ -167,6 +169,7 @@ static ur_result_t redefinedKernelGetGroupInfoAfter(void *pParams) {
return UR_RESULT_SUCCESS;
}

#ifndef _WIN32
static ur_result_t redefinedEnqueueKernelLaunchAfter(void *pParams) {
auto params = *static_cast<ur_enqueue_kernel_launch_params_t *>(pParams);
static ur_event_handle_t UserKernelEvent = **params.pphEvent;
Expand Down Expand Up @@ -197,6 +200,7 @@ static ur_result_t redefinedEventWaitPositive(void *pParams) {
printf("Waiting for events %i, %i\n", EventIdx1, EventIdx2);
return UR_RESULT_SUCCESS;
}
#endif

static ur_result_t redefinedEventWaitNegative(void *pParams) {
auto params = *static_cast<ur_enqueue_events_wait_params_t *>(pParams);
Expand All @@ -223,6 +227,7 @@ static ur_result_t redefinedEnqueueMemBufferMapAfter(void *pParams) {
return UR_RESULT_SUCCESS;
}

#ifndef _WIN32
static void setupMock(sycl::unittest::UrMock<> &Mock) {
using namespace sycl::detail;
mock::getCallbacks().set_after_callback("urKernelGetGroupInfo",
Expand All @@ -234,6 +239,7 @@ static void setupMock(sycl::unittest::UrMock<> &Mock) {
mock::getCallbacks().set_before_callback("urEventWait",
&redefinedEventWaitPositive);
}
#endif

namespace TestInteropKernel {
const sycl::context *Context = nullptr;
Expand Down
Loading