[SYCL] Support dynamic linking on new offloading model #16055
Conversation
maarquitos14
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
maarquitos14
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
| def sycl_allow_device_image_dependencies : Flag<["--", "-"], "sycl-allow-device-image-dependencies">, | ||
| Flags<[WrapperOnlyOption, HelpHidden]>, | ||
| HelpText<"Allow dependencies between device code images">; | ||
| def no_sycl_allow_device_image_dependencies : Flag<["--", "-"], "no-sycl-allow-device-image-dependencies">, |
There was a problem hiding this comment.
I am not sure if we need the negative version when we are dealing with sub-tools
There was a problem hiding this comment.
Let's see what other reviewers think. I don't have a strong opinion either, just added both to be consistent with the driver.
There was a problem hiding this comment.
Just the enabling option for the tools should be enough, the option complexities of what a user can do via the command line warrants both the positive and negative variants there.
asudarsa
left a comment
asudarsa
left a comment
There was a problem hiding this comment.
Looks good to me.
Thanks
maarquitos14
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
maarquitos14
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
…deps-new-offload-driver
maarquitos14
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
maarquitos14
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
maarquitos14
had a problem deploying
to
WindowsCILock
GitHub Actions
Error
maarquitos14
had a problem deploying
to
WindowsCILock
GitHub Actions
Failure
maarquitos14
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
maarquitos14
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
maarquitos14
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
maarquitos14
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
|
@sergey-semenov the review for runtime it's just a few e2e tests copied to use new offload driver, so it should be a quick review :) @asudarsa ping :) |
sarnex
left a comment
sarnex
left a comment
There was a problem hiding this comment.
minor comment on the test
| // RUN: | FileCheck -check-prefix CHECK_DYNAMIC_LINKING %s | ||
| // CHECK_DYNAMIC_LINKING: clang-linker-wrapper{{.*}} "-sycl-allow-device-image-dependencies" | ||
|
|
||
| /// Check for -sycl-allow-device-image-dependencies transmission to clang-linker-wrapper tool |
There was a problem hiding this comment.
It looks like the bottom two cases are ones we should not pass the flag, so maybe we should update the comment?
| /// Check for -sycl-allow-device-image-dependencies transmission to clang-linker-wrapper tool | ||
| // RUN: %clangxx -fsycl -### --offload-new-driver \ | ||
| // RUN: -fno-sycl-allow-device-image-dependencies %s 2>&1 \ | ||
| // RUN: | FileCheck -check-prefix CHECK_NO_DYNAMIC_LINKING %s |
There was a problem hiding this comment.
can you double check and make sure this will actual fail if the arg appears for this case and the below one? if not we might want to use implicit-check-not instead of a check-prefix
There was a problem hiding this comment.
lgtm assuming all the tests are exact copy pastes from the existing tests but with -offload-new-driver added to some commands
note to anyone else: these tests are not intended to be permanent, once we enable the new offload driver by default we can delete all of them
sergey-semenov
left a comment
sergey-semenov
left a comment
There was a problem hiding this comment.
I would prefer if these tests reused the existing source files instead of copying them, but I'm okay with it if the switch to the new model by default is planned to happen soon.
maarquitos14
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
maarquitos14
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
|
@intel/llvm-gatekeepers I think this is ready to merge. |
6 participants
Dynamic linking was already supported in the old offloading model, so bring the support to the new offloading model.