-
Notifications
You must be signed in to change notification settings - Fork 745
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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' | ||
|
@@ -22,6 +25,10 @@ on: | |
description: 'Artifacts retention period' | ||
type: string | ||
default: 3 | ||
compiler: | ||
type: string | ||
required: false | ||
default: "cl" | ||
|
||
outputs: | ||
build_conclusion: | ||
|
@@ -41,6 +48,9 @@ on: | |
type: choice | ||
options: | ||
- "default" | ||
build_configure_extra_args: | ||
type: string | ||
required: false | ||
artifact_archive_name: | ||
type: choice | ||
options: | ||
|
@@ -50,6 +60,12 @@ on: | |
type: choice | ||
options: | ||
- 3 | ||
compiler: | ||
type: choice | ||
options: | ||
- cl | ||
- icx | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will it work without There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we need this to pass the compiler flags we set in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. talked offline, fixed in latest commit, thx |
||
|
||
permissions: read-all | ||
|
||
jobs: | ||
|
@@ -61,37 +77,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 | ||
|
@@ -101,16 +123,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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similarly to CMake options, that's better be handled elsewhere (LIT feature + There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Was There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: | | ||
|
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" | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For other reviewers - looks like we have some tests excluded on There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah tbh i dont know why these dont error with |
||
static int KernelLaunchCounter = KernelLaunchCounterBase; | ||
static constexpr int PauseWaitOnIdx = KernelLaunchCounterBase + 1; | ||
#endif | ||
|
||
// Mock redifinitions | ||
static ur_result_t redefinedKernelGetGroupInfoAfter(void *pParams) { | ||
|
@@ -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; | ||
|
@@ -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); | ||
|
@@ -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", | ||
|
@@ -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; | ||
|
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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