[NewOffloadModel] Add additional options for clang-sycl-linker#21692
[NewOffloadModel] Add additional options for clang-sycl-linker#21692YixingZhang007 wants to merge 17 commits intointel:syclfrom
Conversation
4915195 to
81ce781
Compare
| if (A->getOption().matches(options::OPT_Xlinker)) { | ||
| CmdArgs.push_back(A->getValue()); | ||
| } | ||
| } |
There was a problem hiding this comment.
Why is this needed? I'm seeing values of -Xlinker already being passed as expected.
clang++ --sycl-link --target=spirv64 -### a.o -Xlinker -blah
Intel SYCL compiler development build based on:
clang version 23.0.0git (https://github.com/mdtoguchi/llvm accf054)
Target: spirv64
Thread model: posix
InstalledDir: /localdisk2/mtoguchi/github/llvm/build/bin
Build config: +assertions
"clang-sycl-linker" "a.o" "-blah" "-o" "a.out"
There was a problem hiding this comment.
Thank you for catching this! You're right, the -xlinker option is already handled within AddLinkerInputs function. I've reverted this change :)
| }; | ||
| for (auto [Opt, Flag] : SimpleFlags) | ||
| if (Args.hasArg(Opt)) | ||
| XLinker(Flag); |
There was a problem hiding this comment.
It looks like we have a bit of duplication of option names, etc, which would lead to a maintenance issue with any option name changes or even capturing new flags that need to be forwarded.
Leveraging the Args information would be beneficial here. A new 'Group' can be added for any of these options that need to be forwarded to the clang-sycl-linker. From there, check if an Arg is part of the group and then forward with render()
for (Arg *A : Args.filtered(options::OPT_sycl_Group))
A->render(Args, CmdArgs);
There was a problem hiding this comment.
Hm I am not quiet sure about this because the flags passed to clang-linker-wrapper need to be handled / processed differently when they get passed to clang-sycl-linker (for example option passed into --device-libs= need to be joined by comma). In this case, there might need to be several groups created.
There was a problem hiding this comment.
Sure - This was more of a comment for the general case where options can be directly mapped. Exceptions can be handled differently. Instead of creating a group, just a list of the OPT_ values that do map directly to do the checking, then passing the direct render. At least this way there are less strings to manage.
There was a problem hiding this comment.
Ah, I see! I've created two lists, DirectOpts and ValueOpts, for options that are shared between clang-linker-wrapper and clang-sycl-linker. By calling A->getOption().getName().str(), we can generate the flag names dynamically instead of hardcoding strings, so we avoid maintaining a separate list of hardcoded strings. Please feel free to let me know if anything needed to be changed. Thank you! :)
elizabethandrews
left a comment
There was a problem hiding this comment.
I am not familiar with this and defer to @mdtoguchi for review and approval.
|
|
||
| def input_file_EQ : Joined<["--", "-"], "input-file=">, | ||
| Flags<[LinkerOnlyOption]>, | ||
| HelpText<"Input bitcode file for SYCL device linking">; No newline at end of file |
There was a problem hiding this comment.
Given the text information here - how is this option different than using --bitcode-library? The name of -input-file makes it sound like you can pass any input file and clang-sycl-linker will just know what to do with the file and it not be of any type of file format, so the option name should be more descriptive.
There was a problem hiding this comment.
Thanks for the suggestion. I have changed the option name to be input-bitcode-file= and add a more descriptive comment for both this option as well as --bitcode-library.
| HelpText<"Output in the SYCLBIN binary format in the state specified by <arg> (input, object or executable)">; | ||
|
|
||
| // Options to specify SYCL device kernel bitcode input files, extracted from offload binaries. | ||
| def input_bitcode_file_EQ : Joined<["--", "-"], "input-bitcode-file=">, |
There was a problem hiding this comment.
How are --input-bitcode-file, --bitcode-library and --device-libs used? They all seem to imply the same things.
There was a problem hiding this comment.
--bitcode-library and --device-libs look similar.
--input-bitcode-file looks different.
There was a problem hiding this comment.
I think --input-bitcode-file is not necessary since currently the tool uses OPT_INPUT.
There was a problem hiding this comment.
The --bitcode-library is the same as the --bitcode-library option being passed from clang to clang-linker-wrapper, and the --device-libs are the same as the sycl-device-libraries being passed from clang to clang-linker-wrapper.
--bitcode-librarydirectly injects a raw bitcode file into the SYCL device link stage, filtered by target triple.
--device-libsinjects SYCL runtime library files by filename, resolved from a search directory passed through--library-path=, extracting the correct target's binary from fat offload containers before linking.- Both feed the bitcode library to the same second
llvm-linkstep, but--bitcode-libraryis a simple direct path while--device-libsadds a layer of directory resolution.
I will add comment for these two options to explain the details.
For --input-bitcode-file, I'm thinking of creating it to pass the device input files that are extracted from host fat binaries and converted from SPIR-V to LLVM IR using spirv-to-ir-wrapper. Thanks for the suggestion from Maksim, I agree that we can use the existing OPT_INPUT option and I will remove this --input-bitcode-file
There was a problem hiding this comment.
The use of --sycl-device-libraries should go away. We have moved to linking in the device libraries at compile time and have stopped passing --sycl-device-libraries and --sycl-device-library-location to the clang-linker-wrapper.
maksimsab
left a comment
There was a problem hiding this comment.
Is it possible to add testing?
|
Because I will be leaving this project soon and there is limited time I can work on this PR, I think it would be better to address some of the suggested changes in the comments in a future PR and keep the current PR with the feature we have right now. There are two changes that need to be made in future PRs:
@mdtoguchi @maksimsab Please feel free to let me know if this works, and I will note it in the Jira issue as well. |
Okay by me. One thing to note, however, is that |
This patch is part of the ongoing effort to migrate to clang-sycl-linker. It adds the support for new options required for clang-sycl-linker, where they will be used during the linking and backend compilation stages.
Specifically, the following changes are made:
clang-sycl-linkerare defined inSYCLLinkOpts.td.ClangLinkerWrapper.cpp,appendClangSYCLLinkerArgsis introduced to forward the required flags toclang-sycl-linkervia-XlinkerwhenUseClangSYCLLinkeris enabled.UseClangSYCLLinkeris a temporary guard that will be removed onceclang-sycl-linker` is fully supported.-Xlinkerarguments toclang-sycl-linkerinSPIRV.cpp.