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

[SYCL] Diagnose error in L0 for linker options #5268

Merged
merged 2 commits into from
Jan 14, 2022

Conversation

gmlueck
Copy link
Contributor

@gmlueck gmlueck commented Jan 6, 2022

The Level Zero driver does not support any way to pass options that
are specific to the online linker, so diagnose an error if the
application attempts to pass any. Previously, any such options were
silently ignored.

Note that the Level Zero driver does support online compiler
options, and the plugin correctly handles these.

There is a test case for this PR in intel/llvm-test-suite#713

@gmlueck gmlueck requested a review from smaslov-intel January 6, 2022 19:11
@gmlueck gmlueck requested a review from a team as a code owner January 6, 2022 19:11
@gmlueck gmlueck changed the base branch from private/gmlueck/l0-static-linking3-b to sycl January 7, 2022 21:36
@gmlueck gmlueck requested a review from a team as a code owner January 7, 2022 21:36
The Level Zero driver does not support any way to pass options that
are specific to the online linker, so diagnose an error if the
application attempts to pass any.  Previously, any such options were
silently ignored.

Note that the Level Zero driver does support online _compiler_
options, and the plugin correctly handles these.
@gmlueck gmlueck force-pushed the gmlueck/l0-static-linking3-c branch from 91bd27c to 33bc1b2 Compare January 7, 2022 21:38
@gmlueck
Copy link
Contributor Author

gmlueck commented Jan 7, 2022

This PR is retargeted and rebased now that #5267 is merged.

I expect that the following two E2E tests will fail until intel/llvm-test-suite#713 is merged:

  • SYCL/DeprecatedFeatures/get-options.cpp
  • SYCL/DeprecatedFeatures/program_link.cpp

@bader
Copy link
Contributor

bader commented Jan 10, 2022

I expect that the following two E2E tests will fail until intel/llvm-test-suite#713 is merged:

  • SYCL/DeprecatedFeatures/get-options.cpp
  • SYCL/DeprecatedFeatures/program_link.cpp

CI system is able to test if you expectation holds.

@gmlueck, you can validate that updated DPC++ runtime passes updated tests by adding following comment /verify with <link to PR to test with>.
You can leave such a comment either in this PR (e.g. /verify with https://github.com/intel/llvm-test-suite/pull/713) or in intel/llvm-test-suite#713 (e.g. /verify with https://github.com/intel/llvm/pull/5268).

@gmlueck
Copy link
Contributor Author

gmlueck commented Jan 10, 2022

/verify with intel/llvm-test-suite#713

@gmlueck
Copy link
Contributor Author

gmlueck commented Jan 10, 2022

you can validate that updated DPC++ runtime passes updated tests by adding following comment /verify with .

@bader: thanks for this tip, but I must be doing something wrong. I added a comment like this, but testing does not seem to be re-triggered. What did I do wrong?

@bader
Copy link
Contributor

bader commented Jan 11, 2022

you can validate that updated DPC++ runtime passes updated tests by adding following comment /verify with .

@bader: thanks for this tip, but I must be doing something wrong. I added a comment like this, but testing does not seem to be re-triggered. What did I do wrong?

This comment doesn't re-trigger Jenkins/Precommit, it triggers a new job Jenkins/llvm-test-suite. I see that SYCL/DeprecatedFeatures/get-options.cpp and SYCL/DeprecatedFeatures/program_link.cpp pass, but SYCL :: Reduction/reduction_usm.cpp failed. Tagging @v-klochkov for awareness.

@gmlueck
Copy link
Contributor Author

gmlueck commented Jan 11, 2022

/verify with intel/llvm-test-suite#713

@gmlueck
Copy link
Contributor Author

gmlueck commented Jan 14, 2022

/verify with intel/llvm-test-suite#713

@gmlueck
Copy link
Contributor Author

gmlueck commented Jan 14, 2022

@smaslov-intel : can you review? I have analyzed the failing CI tests, and I think they should not prevent merge:

The "Jenkins/Pre-ci-logs" and "Jenkins/Precommit" failures are expected:

  • DeprecatedFeatures/get-options.cpp
  • DeprecatedFeatures/program_link.cpp

Both of these tests were modified in intel/llvm-test-suite#713, and they will pass once that PR is merged. I verified this with the /verify command in this PR, and you can see that the run of "Jenkins/llvm-test-suite" passed.

The failure in "SYCL / Default Linux / HIP AMD GPU Test Suite" is unrelated to this PR:

  • SubGroup/broadcast.cpp
  • SubGroup/broadcast_fp16.cpp
  • SubGroup/broadcast_fp64.cpp

As evidence, you can see that these same three tests also failed in #4937.

@againull againull merged commit 97e6d2d into intel:sycl Jan 14, 2022
@gmlueck
Copy link
Contributor Author

gmlueck commented Jan 14, 2022

@againull: Thanks for merging! Note that intel/llvm-test-suite#713 also needs to be merged, otherwise the E2E test suite will fail.

@againull
Copy link
Contributor

@againull: Thanks for merging! Note that intel/llvm-test-suite#713 also needs to be merged, otherwise the E2E test suite will fail.

Got it, thank you, merged test as well because 2 PRs were successfully tested together here.

@gmlueck gmlueck deleted the gmlueck/l0-static-linking3-c branch January 18, 2022 22:41
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.

4 participants