Skip to content

[SYCL][E2E] Clean-up test-modes by removing workarounds #17023

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 21 commits into from
Mar 4, 2025

Conversation

ayylol
Copy link
Contributor

@ayylol ayylol commented Feb 14, 2025

  • Removes the build-and-run-mode feature from e2e lit infrastructure, as well as the remaining REQUIRES: build-and-run-mode markups found in tests.
  • Removes the run-only mode ignore line filtering workaround on REQUIRES: build-and-run-mode.
  • Adds build-mode to remove test-mode-run-only mode, this can be tested for by checking run-mode && !build-mode.
  • Marks CPU aot compilation lines with %{run-aux}, as those need to be compiled on a system with the same CPU isa as the running system.

@@ -107,7 +107,7 @@ jobs:
image: ${{ matrix.image }}
image_options: ${{ matrix.image_options }}
target_devices: ${{ matrix.target_devices }}
extra_lit_opts: --param fallback-to-build-if-requires-build-and-run=True ${{ matrix.extra_lit_opts }}
extra_lit_opts: ${{ matrix.extra_lit_opts }}
Copy link
Contributor

Choose a reason for hiding this comment

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

is my understanding that the basic idea of this PR is to merge the build-and-test mode with the run mode, so we build all tests that can't be built in build mode in the run mode?

Copy link
Contributor Author

@ayylol ayylol Feb 14, 2025

Choose a reason for hiding this comment

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

Main motivation is to remove the build-and-run-mode feature, which was just introduced as a way to temporarily mark tests as not available for the split build/run mode while we figured out how to properly react to requires/unsupported statements.

The remaining tests that had this markup (which are all changed in this pr) had not yet had that requirement removed because they either never ran on CI and have regular build failures, or were AOT cpu tests and if we build these on a seperate machine we end up building for that machine's cpu architecture which if it is different than the run system's architecture it causes execution failures.

The fallback-to-build-if-requires-build-and-run option is also removed since that option only functions when a test has REQUIRES: build-and-run-mode

@ayylol
Copy link
Contributor Author

ayylol commented Feb 24, 2025

@intel/bindless-images-reviewers friendly ping

@ayylol ayylol requested a review from przemektmalon February 26, 2025 14:03
@ayylol ayylol temporarily deployed to WindowsCILock March 3, 2025 14:31 — with GitHub Actions Inactive
@ayylol
Copy link
Contributor Author

ayylol commented Mar 3, 2025

@AllanZyne FYI, changed two more asan tests that added REQUIRES: build-and-run-mode since I opened this pr.

@ayylol ayylol temporarily deployed to WindowsCILock March 3, 2025 15:00 — with GitHub Actions Inactive
@AllanZyne
Copy link
Contributor

@AllanZyne FYI, changed two more asan tests that added REQUIRES: build-and-run-mode since I opened this pr.

Got it. Thanks.

@ayylol
Copy link
Contributor Author

ayylol commented Mar 4, 2025

@intel/llvm-gatekeepers this is ready to merge.

@kbenzie kbenzie merged commit 5ab196d into intel:sycl Mar 4, 2025
21 checks passed
@ayylol ayylol deleted the remove-build-and-run branch March 4, 2025 14:22
@kbenzie
Copy link
Contributor

kbenzie commented Mar 4, 2025

There are post commit failures from merging this https://github.com/intel/llvm/actions/runs/13655785872 @ayylol

@kbenzie
Copy link
Contributor

kbenzie commented Mar 4, 2025

I don't see any existing issues for these failing in post commit:

GEN12 on Linux:

Failed Tests (3):
  SYCL :: AOT/early_aot.cpp
  SYCL :: BFloat16/bfloat16_example_aot_gpu.cpp
  SYCL :: SpecConstants/2020/image_selection.cpp

Arc on Linux:

Failed Tests (2):
  SYCL :: AOT/early_aot.cpp
  SYCL :: SpecConstants/2020/image_selection.cpp

@ayylol
Copy link
Contributor Author

ayylol commented Mar 4, 2025

Arc on Linux:

Failed Tests (2):
  SYCL :: AOT/early_aot.cpp
  SYCL :: SpecConstants/2020/image_selection.cpp

The two ARC failures are present in the commit immediately prior to mine https://github.com/intel/llvm/actions/runs/13654810542/job/38173816553 ef28856

Let me take a closer look at these.

@kbenzie
Copy link
Contributor

kbenzie commented Mar 4, 2025

The two ARC failures are present in the commit immediately prior to mine https://github.com/intel/llvm/actions/runs/13654810542/job/38173816553 ef28856

That commit only touched different tests and docs so seems like it shouldn't have caused the regerssions. Did the containers get updated recently?

@sarnex
Copy link
Contributor

sarnex commented Mar 4, 2025

They got updated in the commit after this here, depending on timing it's possible the container updating job there ran before the E2E test job for this commit or the previous one.

If you believe the issue is the driver update please tell me, it's my problem then.

@ayylol
Copy link
Contributor Author

ayylol commented Mar 4, 2025

If you believe the issue is the driver update please tell me, it's my problem then.

Would you be able to look into this? I think the fact that this pr did not touch any of those tests, and neither did the previous pr points to that likely being the case. I also briefly tried running the AOT/early_aot.cpp test locally on a gen12 machine outside of a container and couldnt reproduce the failure.

@sarnex
Copy link
Contributor

sarnex commented Mar 4, 2025

Ok It was probably a timing issue with the container update, I'll take a look.

@sarnex
Copy link
Contributor

sarnex commented Mar 4, 2025

#17306 for now

jchlanda pushed a commit to jchlanda/llvm that referenced this pull request Mar 6, 2025
- Removes the `build-and-run-mode` feature from e2e lit infrastructure,
as well as the remaining `REQUIRES: build-and-run-mode` markups found in
tests.
- Removes the `run-only` mode ignore line filtering workaround on
`REQUIRES: build-and-run-mode`.
- Adds `build-mode` to remove `test-mode-run-only` mode, this can be
tested for by checking `run-mode && !build-mode`.
- Marks CPU aot compilation lines with `%{run-aux}`, as those need to be
compiled on a system with the same CPU isa as the running system.
AlexeySachkov pushed a commit to AlexeySachkov/llvm that referenced this pull request Apr 2, 2025
- Removes the `build-and-run-mode` feature from e2e lit infrastructure,
as well as the remaining `REQUIRES: build-and-run-mode` markups found in
tests.
- Removes the `run-only` mode ignore line filtering workaround on
`REQUIRES: build-and-run-mode`.
- Adds `build-mode` to remove `test-mode-run-only` mode, this can be
tested for by checking `run-mode && !build-mode`.
- Marks CPU aot compilation lines with `%{run-aux}`, as those need to be
compiled on a system with the same CPU isa as the running system.
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.

8 participants