[SYCL] Add option forwarding from clang-linker-wrapper tool to clang-sycl-linker #21958
[SYCL] Add option forwarding from clang-linker-wrapper tool to clang-sycl-linker #21958srividya-sundaram wants to merge 1 commit intointel:syclfrom
Conversation
| return createStringError("Could not link IR"); | ||
| } | ||
|
|
||
| // Get all SYCL device library files, if any. |
There was a problem hiding this comment.
I think this clean up should be submitted upstream. But before it is submitted upstream, I think we need to submit to upstream importing of device libraries in code-gen stage, similar to what we did in downstream.
Otherwise, functionality between community clang-sycl-linker and downstream clang-sycl-linker diverges a lot and it will be incrementally more and more difficult to maintain 2 variants of code.
There was a problem hiding this comment.
So,
- Submit PR21672 upstream.
- Submit
--device-libsand--library-pathcleanup upstream.
Is this correct understanding?
If yes, I can work on # 1 and # 2 submissions to upstream, and we can split this PR to have just the option forwarding + test changes and add the device libs cleanup after the upstream patches land.
WDYT?
There was a problem hiding this comment.
Yes, that sounds correct to me. I don't know how 1. would look like in upstream (unlikely can be submitted as-is), since it seems there might be some functionality missing yet or code is different, but it would be the best way to move forward in my opinion.
|
More general comment.
In cases like that the suggested approach is: https://llvm.org/docs/Contributing.html#how-to-submit-a-patch Please, use this approach in your patches. |
srividya-sundaram
force-pushed
the
clw-to-csl
branch
from
8d6725d to
20f67e1
Compare
YuriPlyakhin
left a comment
YuriPlyakhin
left a comment
There was a problem hiding this comment.
could you please update title?
srividya-sundaram
changed the title
srividya-sundaram
changed the title
|
|
||
| if (UseClangSYCLLinker) { | ||
| CmdArgs.push_back("-fsycl"); | ||
| CmdArgs.push_back("--sycl-link"); |
There was a problem hiding this comment.
I looked at implementation of clang function of ClangLinkerWrapper.cpp in downstream and upstream. I see some divergence, but at the same time there are a lot of same staff. For example, we already pass CmdArgs.push_back("--sycl-link");, in upstream, just slightly differently. Is it possible to align downstream code to upstream as much as possible (maybe as a prerequisite patch to this patch, or maybe as part of this patch), and mark customized parts with some marker? maybe // INTEL_CUSTOMIZATION begin/end?
it would be good to also start using ActiveOffloadKindMask
Since we are going to modify same places in upstream and downstream, I think it is important to keep them as synced as possible.
Also, partially this patch already aligns downstream to upstream, my ask is to just do it in a more complete form.
Also, it might happen that part of this patch we also could submit upstream, instead of downstream, but it is difficult to say for sure now, because of the divergence in code. When the code is more aligned, we can check again.
Does it make sense?
There was a problem hiding this comment.
mark customized parts with some marker? maybe
// INTEL_CUSTOMIZATION begin/end?
I see no reasons for adding "markers". Why would you want that?
4 participants
This is the first patch in a series that moves SYCL device code linking logic from
clang-linker-wrapperinto theclang-sycl-linkertool.I’m continuing the work from where Yixing left off.
At the moment, the tests only verify that the options are forwarded to
clang-linker-wrapper.Tests validating
clang-sycl-linkerinvocations will be added in subsequent PRs.Added infrastructure from PR 21692 + tests for option forwarding.