Skip to content

[CI] Add backward ABI-compatibility testing to pre-commit #19719

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Aug 7, 2025
Merged

[CI] Add backward ABI-compatibility testing to pre-commit #19719

merged 6 commits into from
Aug 7, 2025
+110 −8

Conversation

aelovikov-intel
Copy link
Contributor

No description provided.

@aelovikov-intel aelovikov-intel force-pushed the ci-abi-compat branch 4 times, most recently from 30a6942 to 97b4425 Compare August 6, 2025 15:11
@aelovikov-intel aelovikov-intel requested a review from a team as a code owner August 7, 2025 15:57
uditagarwal97
uditagarwal97 reviewed Aug 7, 2025
View reviewed changes
sarnex
sarnex approved these changes Aug 7, 2025
View reviewed changes
aelovikov-intel
aelovikov-intel commented Aug 7, 2025
View reviewed changes
@aelovikov-intel aelovikov-intel merged commit efe036e into intel:sycl Aug 7, 2025
36 of 37 checks passed
@aelovikov-intel aelovikov-intel deleted the ci-abi-compat branch August 7, 2025 18:30
aelovikov-intel added a commit that referenced this pull request Aug 7, 2025
@aelovikov-intel
[CI] Use E2E container binaries in run-only manual E2E task dispatch
55b2bcb 
#19719 broke `workflow_dispatch` mode
for manual E2E task invocation for compatibility testing as we don't
have the required `binaries_artifact` input in that mode. Github limits
number of input parameters in `workflow_dispatch` mode, so restore the
functionality without introducing extra input.
aelovikov-intel added a commit that referenced this pull request Aug 7, 2025
@aelovikov-intel
[CI] Use E2E container binaries in run-only manual E2E task dispatch
8cae55c 
#19719 broke `workflow_dispatch` mode
for manual E2E task invocation for compatibility testing as we don't
have the required `binaries_artifact` input in that mode. Github limits
number of input parameters in `workflow_dispatch` mode, so restore the
functionality without introducing extra input.
@aelovikov-intel aelovikov-intel mentioned this pull request Aug 7, 2025
Merged
aelovikov-intel added a commit that referenced this pull request Aug 7, 2025
@aelovikov-intel
[CI] Use E2E container binaries in run-only manual E2E task dispatch (
2fac5a7 
#19744)

#19719 broke `workflow_dispatch` mode
for manual E2E task invocation for compatibility testing as we don't
have the required `binaries_artifact` input in that mode. Github limits
number of input parameters in `workflow_dispatch` mode, so restore the
functionality without introducing extra input.
aelovikov-intel
aelovikov-intel commented Aug 11, 2025
View reviewed changes
devops/compat_ci_exclude.sycl-rel-6_2

# Need more investigation by the author:

# https://github.com/intel/llvm/pull/18314
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Alexandr-Konovalov , looks like your #18314 broke backward compatibility here, see
https://github.com/intel/llvm/actions/runs/16882134385 (using nightly-2025-05-08, pass)
https://github.com/intel/llvm/actions/runs/16882361500 (using nightly-2025-05-09, fail)
Please investigate and either fix (might need backporting to sycl-rel-6_3) or create a PR updating this file describing why it wasn't an ABI-break or was allowed/desired/approved.

aelovikov-intel
aelovikov-intel commented Aug 11, 2025
View reviewed changes
devops/compat_ci_exclude.sycl-rel-6_2
Assert/assert_in_simultaneously_multiple_tus.cpp
Assert/assert_in_simultaneously_multiple_tus_one_ndebug.cpp

# https://github.com/intel/llvm/pull/17442
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@steffenlarsen , looks like your #17442 broke backward compatibility here, see
https://github.com/intel/llvm/actions/runs/16882650449 (using nightly-2025-05-01, pass)
https://github.com/intel/llvm/actions/runs/16882716814 (using nightly-2025-05-02, fail)
Please investigate and either fix (might need backporting to sycl-rel-6_3) or create a PR updating this file describing why it wasn't an ABI-break or was allowed/desired/approved.

Copy link
Contributor

@steffenlarsen steffenlarsen Aug 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Will do! Great addition, by the way!

See #19764.

aelovikov-intel
aelovikov-intel commented Aug 11, 2025
View reviewed changes
devops/compat_ci_exclude.sycl-rel-6_2
NewOffloadDriver/split-per-source-main.cpp
NewOffloadDriver/sycl-external-with-optional-features.cpp

# https://github.com/intel/llvm/pull/18277
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@igchor , looks like your #18277 broke backward compatibility here, see

https://github.com/intel/llvm/actions/runs/16883391382 (using nightly-2025-05-19, pass)
https://github.com/intel/llvm/actions/runs/16883503382 (using nightly-2025-05-23, fail)
Please investigate and either fix (might need backporting to sycl-rel-6_3) or create a PR updating this file describing why it wasn't an ABI-break or was allowed/desired/approved.

aelovikov-intel
aelovikov-intel commented Aug 11, 2025
View reviewed changes
devops/compat_ci_exclude.sycl-rel-6_2
Comment on lines +74 to +75
# https://github.com/intel/llvm/pull/19328
Adapters/interop-level-zero-buffer-ownership.cpp
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@igchor , looks like your #19328 broke backward compatibility here, see

https://github.com/intel/llvm/actions/runs/16883683175 (using nightly-2025-07-10, pass)
https://github.com/intel/llvm/actions/runs/16883690547 (using nightly-2025-07-11, fail)
Please investigate and either fix (might need backporting to sycl-rel-6_3) or create a PR updating this file describing why it wasn't an ABI-break or was allowed/desired/approved.

aelovikov-intel
aelovikov-intel commented Aug 11, 2025
View reviewed changes
devops/compat_ci_exclude.sycl-rel-6_2
Comment on lines +56 to +68
# https://github.com/intel/llvm/pull/18403 (pulldown, so probably offload-tools
# team should be looking into this)
DeviceImageDependencies/NewOffloadDriver/dynamic.cpp
DeviceImageDependencies/NewOffloadDriver/free_function_kernels.cpp
DeviceImageDependencies/NewOffloadDriver/math_device_lib.cpp
DeviceImageDependencies/NewOffloadDriver/objects.cpp
DeviceImageDependencies/NewOffloadDriver/singleDynamicLibrary.cpp
NewOffloadDriver/aot-gpu.cpp
NewOffloadDriver/buffer.cpp
NewOffloadDriver/multisource.cpp
NewOffloadDriver/spirv_device_obj_smoke.cpp
NewOffloadDriver/split-per-source-main.cpp
NewOffloadDriver/sycl-external-with-optional-features.cpp
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@intel/dpcpp-tools-reviewers , looks like #18403 pulldown broke backward compatibility here

https://github.com/intel/llvm/actions/runs/16883874414 (using nightly-2025-05-16, pass)
https://github.com/intel/llvm/actions/runs/16883880550 (using nightly-2025-05-17, fail)
Please investigate and either fix (might need backporting to sycl-rel-6_3) or create a PR updating this file describing why it wasn't an ABI-break.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was suggested to me to tag @asudarsa here.

aelovikov-intel
aelovikov-intel commented Aug 11, 2025
View reviewed changes
devops/compat_ci_exclude.sycl-rel-6_2
Comment on lines +18 to +24
# https://github.com/intel/llvm/pull/18565
# This one should have probably be done in multiple PRs, first improving the
# test's CHECKs and then making an actual functional change.
#
# Based on the title, I'd expect to see reduction in number of ur*retain/release
# calls, but it only moves a few ur*release CHECKs, so I'll let the author
# update this explanation.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Alexandr-Konovalov , please upload a PR that updates this comment with explanation how the change wasn't ABI-breaking.

aelovikov-intel
aelovikov-intel commented Aug 11, 2025
View reviewed changes
devops/compat_ci_exclude.sycl-rel-6_2
# binaries built with sanitizers?
Sanitizer

# https://github.com/intel/llvm/pull/19238
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@AndreiZibrov , please upload a PR that updates this with an explanation why this was desired (I'd expect something along the lines of allowing ABI breaks in an experimental extension).

Also tagging @AlexeySachkov

Copy link
Contributor

Choose a reason for hiding this comment

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

@aelovikov-intel - Do you have the output of this failure at hand?

aelovikov-intel
aelovikov-intel commented Aug 11, 2025
View reviewed changes
devops/compat_ci_exclude.sycl-rel-6_2
Comment on lines +35 to +38
# https://github.com/intel/llvm/pull/17955, experimental extension
AsyncAlloc/device/async_alloc_from_pool.cpp
AsyncAlloc/device/async_alloc_zero_init.cpp
AsyncAlloc/device/ooo_queue_async_alloc_from_pool.cpp
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@intel/sycl-graphs-reviewers , can you please check if that break was intentional and if so update the comment here?

Using 2025-06-18 (pass): https://github.com/intel/llvm/actions/runs/16884717829
Using 2025-06-19 (fail): https://github.com/intel/llvm/actions/runs/16884725131

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@steffenlarsen steffenlarsen steffenlarsen left review comments

@sarnex sarnex sarnex approved these changes

@uditagarwal97 uditagarwal97 uditagarwal97 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@aelovikov-intel @sarnex @uditagarwal97 @steffenlarsen