[SYCL][NewOffloadModel] Fix -Xdevice-post-link, -Xspirv-translator and -Xspirv-to-ir-wrapper#22664
[SYCL][NewOffloadModel] Fix -Xdevice-post-link, -Xspirv-translator and -Xspirv-to-ir-wrapper#22664sarnex wants to merge 1 commit into
Conversation
sarnex
changed the title
sarnex
changed the title
… and -Xspirv-translator Signed-off-by: Nick Sarnie <nick.sarnie@intel.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| }; | ||
| // --sycl-post-link-options="options" provides a string of options to be | ||
| // passed along to the sycl-post-link tool during device link. | ||
| // Xdevice-post-link is processed separately later. |
There was a problem hiding this comment.
nit: Instead of having " is processed separately later" can we put like a comment section toward the end (or beginning) that will say what flags are processed later? This way we have a common one place to search?
| "--spirv-builtin-format=global"); | ||
| for (const Arg *A : Args.filtered(OPT_spirv_to_ir_wrapper_arg_EQ)) { | ||
| StringRef(A->getValue()) | ||
| .split(CmdArgs, " ", /* MaxSplit = */ -1, /* KeepEmpty = */ false); |
There was a problem hiding this comment.
nit: Does the space between = work? I thought it needs to be no spaces so that the documentation tool can pick it up.
| TC->getTriple(), Args, BuildArgs, options::OPT_Xdevice_post_link, | ||
| options::OPT_Xdevice_post_link_EQ, /*Device=*/StringRef()); | ||
| for (const auto &A : BuildArgs) | ||
| appendOption(PostLinkOptString, A); |
There was a problem hiding this comment.
Alexey is working on other issues with how we pass along arguments to the clang-linker-wrapper, especially when dealing with multiple arguments: #22645
Can we do something similar with the --sycl-post-link-options, --llvm-spirv-options and '--spirv-to-ir-wrapper-options` options to where we can pass the options multiple times with an argument each time to tokenize the args instead of passing a single string?
| // Pass backend compiler, linker options, sycl-post-link options, | ||
| // llvm-spirv options and spirv-to-ir-wrapper options specified at link | ||
| // time to clang-linker-wrapper. Link-time options passed via | ||
| // -Xsycl-target-backend are forwarded using --device-compiler, options | ||
| // passed via -Xsycl-target-linker are forwarded using --device-linker, | ||
| // options passed via -Xdevice-post-link are forwarded using | ||
| // --sycl-post-link-options, options passed via -Xspirv-translator are | ||
| // forwarded using --llvm-spirv-options, and options passed via | ||
| // -Xspirv-to-ir-wrapper are forwarded using | ||
| // --spirv-to-ir-wrapper-options. |
There was a problem hiding this comment.
nit.
| // Pass backend compiler, linker options, sycl-post-link options, | |
| // llvm-spirv options and spirv-to-ir-wrapper options specified at link | |
| // time to clang-linker-wrapper. Link-time options passed via | |
| // -Xsycl-target-backend are forwarded using --device-compiler, options | |
| // passed via -Xsycl-target-linker are forwarded using --device-linker, | |
| // options passed via -Xdevice-post-link are forwarded using | |
| // --sycl-post-link-options, options passed via -Xspirv-translator are | |
| // forwarded using --llvm-spirv-options, and options passed via | |
| // -Xspirv-to-ir-wrapper are forwarded using | |
| // --spirv-to-ir-wrapper-options. | |
| // Pass backend compiler, linker, sycl-post-link, | |
| // llvm-spirv, and spirv-to-ir-wrapper options specified at link | |
| // time to clang-linker-wrapper, using the following mapping: | |
| // -Xsycl-target-backend -> --device-compiler | |
| // -Xsycl-target-linker -> --device-linker | |
| // -Xdevice-post-link -> --sycl-post-link-options | |
| // -Xspirv-translator -> --llvm-spirv-options | |
| // -Xspirv-to-ir-wrapper -> --spirv-to-ir-wrapper-options. |
| // Special option to pass in sycl-post-link options. Accepts the | ||
| // [<kind>:][<triple>=]<value> form so options can be scoped to a target; they | ||
| // are filtered per-triple into -sycl-post-link-arg=. |
There was a problem hiding this comment.
nit.
Do we actually need such detailed description in comments?
I'm thinking the option name, metavarname and help text should be descriptive enough to understand the option without additional comments.
I would remove it here and in other places in .td file and instead if metavarname or helptext is not descriptive enough, maybe it is better to update those.
| // Special option to pass in sycl-post-link options. Accepts the | |
| // [<kind>:][<triple>=]<value> form so options can be scoped to a target; they | |
| // are filtered per-triple into -sycl-post-link-arg=. |
| }; | ||
| // --sycl-post-link-options="options" provides a string of options to be | ||
| // passed along to the sycl-post-link tool during device link. | ||
| // Xdevice-post-link is processed separately later. |
There was a problem hiding this comment.
nit
| // Xdevice-post-link is processed separately later. | |
| // -Xdevice-post-link is processed separately later. |
| } | ||
|
|
||
| BuildArgs.clear(); | ||
| SmallString<128> PostLinkOptString; |
There was a problem hiding this comment.
PostLinkOptString here shadows the outer PostLinkOptString declared above.
Maybe:
| SmallString<128> PostLinkOptString; | |
| SmallString<128> PerTargetPostLinkOptString; |
YuriPlyakhin
left a comment
YuriPlyakhin
left a comment
There was a problem hiding this comment.
Overall, LGTM, thank you! Some nits/comments.
4 participants
We had some tests failing in the new offload model because they were doing:
The
-Xspirv-translator=form wasn't implemented, so implement it.Also it seems no tests are doing it but we had the same problem with
-Xdevice-post-link=, and-Xspirv-to-ir-wrapperwasn't implemented at all, so implement those too.Closes: #22372
CMPLRLLVM-76314
CMPLRLLVM-68973
Co-Authored-By: Claude Opus 4.8 (1M context) noreply@anthropic.com