[sycl][NewOffloadModel]Init commit for moving device-compiler handling to clang-sycl-linker.#22254
[sycl][NewOffloadModel]Init commit for moving device-compiler handling to clang-sycl-linker.#22254bviyer wants to merge 5 commits into
Conversation
bviyer
force-pushed
the
bviyer-be-parsing-to-clang-sycl-ld
branch
from
933abbc to
5ef7006
Compare
bviyer
force-pushed
the
bviyer-be-parsing-to-clang-sycl-ld
branch
from
8429b97 to
a41ff48
Compare
Otherwise driver thinks these flags are unused.
bviyer
changed the title
before using its value in the range-based for. I think this is the cause of an issue in windows CI.
|
pls, use SYCL - all capital letters in PR title. |
YuriPlyakhin
left a comment
YuriPlyakhin
left a comment
There was a problem hiding this comment.
It seems some CI failures are related to this PR, could you please check?
YuriPlyakhin
left a comment
YuriPlyakhin
left a comment
There was a problem hiding this comment.
We don't want to support -fsycl-targets=spir64_gen in clang driver for new offloading model. Could you please update PR description and PR itself, if needed.
YuriPlyakhin
left a comment
YuriPlyakhin
left a comment
There was a problem hiding this comment.
I'm a bit confused by PR title and description.
Title says that this PR is about moving of device-compiler handling to clang-sycl-linker. Yet, in PR description, it highlights what will be passed to clang-linker-wrapper and says nothing about clang-sycl-linker.
Could you please clarify?
| SYCLTC.TranslateBackendTargetArgs(TC->getTriple(), Args, BuildArgs); | ||
| for (const auto &A : BuildArgs) | ||
| appendOption(BackendOptString, A); | ||
| const llvm::Triple &TCTriple = TC->getTriple(); |
There was a problem hiding this comment.
Could you please add to PR description some summary, of what this change to clang.cpp is achieving?
maybe "before" and "after" states.
Also, if the behavior of clang driver is changed, I would expect some lit test changes required. Probably some new driver tests should be added?
| #include "clang/Driver/CommonArgs.h" | ||
| #include "clang/Driver/Compilation.h" | ||
| #include "clang/Driver/Driver.h" | ||
| #include "clang/Options/Options.h" |
There was a problem hiding this comment.
It seems quite a lot of things are happening in this PR.
Is it possible to split it into smaller chunks (for example separate refactoring from functional changes or split changes to different tools into different PRs if it makes sense)?
Could you please add a bit more details into description for each meaningful change.
And I see a lot of changes in SYCL.cpp, but no driver tests affected. Is it solely NFC refactoring or do we just have a gap in our testing?
There was a problem hiding this comment.
And maybe the most important feedback:
could you please provide design doc or something that would explain what we want to achieve by this move? What's the current state, why it is a problem and what we will have as a result of this move. And how it aligns SYCL with how device-compiler and device-linker options are handled for other programming models and targets in clang-linker-wrapper in upstream LLVM.
Maybe such document already exists, but I don't remember from the top of my head.
| ; Test that IMG_Object image kind is set for AOT compilation (Intel CPU). | ||
| ; RUN: llvm-objdump --offloading %t/aot-cpu.out | FileCheck %s --check-prefix=IMAGE-KIND-OBJECT | ||
| ; | ||
| ; --device-compiler=[<kind>:][<triple>=]<value> reaches ocloc only when the |
There was a problem hiding this comment.
hmm, I thought the plan was to use ocloc_options_EQ for that in clang-sycl-linker.
But I guess this confusion is related to my previous comment about need for some design doc that would explain the direction we are trying to achieve by this pr.
2 participants
--device-compilerhandling is moved from clang to clang-sycl-linker.For example, when the user passes the following flags into clang++ on a x86_64 host:
The clang-linker-wrapper will get the following:
NOTE: To avoid IP/Path leaks, certain fields - that are irrelevant for this discussion - are replaced with