Skip to content

Conversation

@VyacheslavLevytskyy
Copy link
Contributor

@VyacheslavLevytskyy VyacheslavLevytskyy commented Dec 10, 2024

Introduce SPIR-V Backend support in SYCL tests and end-to-end tests by adding XFAIL & UNSUPPORTED to tests that are not passed at the moment when Khronos Translator is substituted by SPIR-V Backend. Three syntax-only tests are moved from e2e to tests.

@VyacheslavLevytskyy
Copy link
Contributor Author

@AlexeySachkov @sarnex Please have a look. This PR shows how we possibly can enable the LLVM SPIR-V Backend in DPC++ CI. A separate question is how to pass "-fsycl-use-spirv-backend-for-spirv-gen" in the SYCL CTS test suite.

Copy link
Contributor

@AlexeySachkov AlexeySachkov left a comment

Choose a reason for hiding this comment

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

sycl/test-e2e changes LGTM

Comment on lines 132 to 146
Copy link
Contributor

Choose a reason for hiding this comment

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

I have no objections to that conceptually, but I have a concern that we may not have enough HW available to finish that testing in reasonable time,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I agree. I will keep the PR draft until we have completed discussing changes and potential consequences.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah especially since we're using gen12 which takes forever to run as it is. can you bring this up at our next gatekeeper/CI meeting? there is one today but idk if there is time for this issue. will fwd.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, thank you

Copy link
Contributor

@sarnex sarnex left a comment

Choose a reason for hiding this comment

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

needs discussion at CI meeting imo

@VyacheslavLevytskyy
Copy link
Contributor Author

needs discussion at CI meeting imo

@sarnex What do you think about the following way forward -- I will remove SYCL CTS from .github/workflows/sycl-nightly.yml, so we have just E2E testing turned on? In this case the blocker for this PR can be addressed separately in the next PR, after we discuss at the CI meeting, and we will be able to proceed without longer pauses.

@sarnex
Copy link
Contributor

sarnex commented Dec 10, 2024

just E2E in the nightly should be fine IMO, it runs in the middle of the night when nobody is working and CI is empty

but i will ask you to check the results evyery day and make sure its passing and fix/disable failing tests

Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally, -fsyntax-only check should be moved into check-sycl. That's not a strong comment in this case, because it is a "combined" test (i.e. syntax + execution), but still

@VyacheslavLevytskyy VyacheslavLevytskyy force-pushed the introduce_SPIRV_Backend branch 2 times, most recently from 36072bb to c75b88d Compare January 8, 2025 14:17
image: ghcr.io/intel/llvm/ubuntu2404_intel_drivers:latest
image_options: -u 1001 --device=/dev/dri --privileged --cap-add SYS_ADMIN
target_devices: opencl:cpu
extra_lit_opts: -j 50 --param spirv-backend=True
Copy link
Contributor

Choose a reason for hiding this comment

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

This makes no sense, these runners have desktop CPUs and you'll oversubscribe them.

image_options: -u 1001 --privileged --cap-add SYS_ADMIN
target_devices: opencl:cpu
tests_selector: e2e
extra_lit_opts: --param spirv-backend=True --param gpu-intel-gen12=True
Copy link
Contributor

Choose a reason for hiding this comment

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

gpu-intel-gen12 isn't required after #16220.

Copy link
Contributor

@aelovikov-intel aelovikov-intel left a 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 be split into several patches:

  1. sycl-linux-run-tests.yml so that the testing can be dispatched manually.
  2. Update tests and verify via manual CI runs
  3. CI enabling in pre-commit/nightly. Shouldn't include many of the tests changes at that poing.

I also see some supposedly functional changes, they should go into separate PR as well.

Copy link
Contributor

@YuriPlyakhin YuriPlyakhin left a comment

Choose a reason for hiding this comment

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

Joint Matrix change LGTM

Copy link
Contributor

@dkhaldi dkhaldi left a comment

Choose a reason for hiding this comment

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

JM changes LGTM

@VyacheslavLevytskyy VyacheslavLevytskyy changed the title [E2E][CI][CTS] Introduce SPIR-V Backend support in LIT tests and CI [E2E] Introduce SPIR-V Backend support in SYCL tests and end-to-end tests Jan 13, 2025
@VyacheslavLevytskyy
Copy link
Contributor Author

It was agreed that we split #16317 into three parts:

  • CI
  • Clang driver
  • XFAIL & UNSUPPORTED in end-to-end tests.

I reverted changes in CI/Clang here, so that this PR is only about introducing XFAIL & UNSUPPORTED into end-to-end tests now.

@sarnex Can you please have a look and unblock if everything is ok

@aelovikov-intel
Copy link
Contributor

aelovikov-intel commented Jan 13, 2025

@sarnex Can you please have a look and unblock if everything is ok

IMO, this PR shouldn't even be outside draft state until #16600 is merged in.

Copy link
Contributor

@joeatodd joeatodd left a comment

Choose a reason for hiding this comment

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

syclcompat changes LGTM

@VyacheslavLevytskyy
Copy link
Contributor Author

@sarnex @aelovikov-intel We have #16600 and #16602 merged. This PR contains only changes in test cases, and we got them approved as well. Are we good to proceed with a merge of this PR?

Copy link
Contributor

@sarnex sarnex left a comment

Choose a reason for hiding this comment

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

lgtm just typos

@aelovikov-intel
Copy link
Contributor

@sarnex @aelovikov-intel We have #16600 and #16602 merged. This PR contains only changes in test cases, and we got them approved as well. Are we good to proceed with a merge of this PR?

I'd like to see some CI results before doing that. Are we in a state when that can be done with manual dispatch of "SYCL E2E" task?

@VyacheslavLevytskyy
Copy link
Contributor Author

@sarnex @aelovikov-intel We have #16600 and #16602 merged. This PR contains only changes in test cases, and we got them approved as well. Are we good to proceed with a merge of this PR?

I'd like to see some CI results before doing that. Are we in a state when that can be done with manual dispatch of "SYCL E2E" task?

Thank you for looking into it. However, I disagree that this is a prerequisite to merge this PR.

  • There is no need to run CI at this time. We have results of workflows runs (sycl-nightly.yml and sycl-linux-precommit.yml) from Jan 10, where all was green and all changes in end-to-end tests were verified. There is no sense in races between my updates to e2e tests and routine regular changes in intel/llvm. You will not get actual results until you will be ready to run new (not yet created) CI actions, and you already have fresh enough results from last Friday.
  • We should postpone any additional runs & checks if we have enough XFAIL & UNSUPPORTED for SPIR-v Backend to stay green until after the moment when CI itself is ready to be updated. This PR doesn't change any yml specifications, as we explicitly discussed and agreed upon, and the whole sense of removing from the initial PR changes in .github/workflows/sycl-nightly.yml & .github/workflows/sycl-linux-precommit.yml was to avoid any prerequisites of such kind for this PR.
  • It's not the last time when those e2e tests are being updated due to progress in development of SPIR-V Backend, drivers update and pulldowns. This is a continuous activity, and, being a process rather than an event, it can't be a blocker to merge this PR.

I'd propose to merge this PR without any additional verifications. Results from January 10 works as well until we get Github Actions updated and ready to run.

Meanwhile, I appreciate your suggestion to use actions/workflows/sycl-linux-run-tests.yml, and without any doubts will use this tool to establish a proper process of syncing SPIR-V Backend progress with SYCL end-to-end test suite.

Copy link
Contributor

@aelovikov-intel aelovikov-intel left a comment

Choose a reason for hiding this comment

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

@aelovikov-intel aelovikov-intel merged commit 1bd15f0 into intel:sycl Jan 16, 2025
17 checks passed
@VyacheslavLevytskyy
Copy link
Contributor Author

Before this PR: https://github.com/intel/llvm/actions/runs/12814470669 (581 fails) With this PR: https://github.com/intel/llvm/actions/runs/12815598260 (4 fails)

Thank you. I will update those prior to CI is updated.

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.

9 participants