[SYCL][NewOffloadModel] Fix -Xdevice-post-link, -Xspirv-translator and -Xspirv-to-ir-wrapper#22664
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?
There was a problem hiding this comment.
Sorry do you mind giving an example, I wasn't able to follow, thanks
There was a problem hiding this comment.
I think you have inline comments saying "Xdevice-post-link is processed separately later." and later on you have another comment saying " is processed separately later." Instead of this, toward the end of this list, have a section saying: "Following flags will be processed at a later section:"
This way, we have one place to look for all the flags that will be processed later.
This is not a hard requirement from me :). Just that it would be easier to understand things - one place to look - for a noobie like me.
There was a problem hiding this comment.
Ok thanks, I added the Xdevice-post-link is processed separately later. comment to the --sycl-post-link-options section because both concern options to the sycl-post-link tool, and similarly for -Xspirv-translator is processed separately later. it's in the section processing -llvm-spirv-options, both of which pass options to the llvm-spirv tool, so my idea was people reading the respective section's code might want to know the other place where options to the respective tool are coming from. Let me know if I misunderstood the suggestion
| "--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.
There was a problem hiding this comment.
Both should be fine, I've never heard about having to change inline comments in a function call because of doxygen/sphinx
| 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?
There was a problem hiding this comment.
Sure, would it be okay with you if I did that in a follow-up commit? I can make a Jira to investigate the current status and implement behavior like #22645 in cases where it does not match. Thanks
There was a problem hiding this comment.
Follow up commit is fine by me.
There was a problem hiding this comment.
Thanks, made CMPLRLLVM-77053 so I can't weasel my way out of it
There was a problem hiding this comment.
@sarnex, please, forward each driver option to clang-linker-wrapper as a separate option. Currently, driver joins all options into a single string using space an option separator and clang-linker-wrapper tries to undo this joining by splitting the string. If any option value uses space as part of the option value, we have a bug.
There was a problem hiding this comment.
I'm already working on this PR, thanks
YuriPlyakhin
left a comment
YuriPlyakhin
left a comment
There was a problem hiding this comment.
Overall, LGTM, thank you! Some nits/comments.
Signed-off-by: Nick Sarnie <nick.sarnie@intel.com>
Signed-off-by: Nick Sarnie <nick.sarnie@intel.com>
|
@intel/llvm-gatekeepers please consider merging |
8 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