-
Notifications
You must be signed in to change notification settings - Fork 803
[NewOffloadModel] Pass link-time options through device-compiler and device-linker argument for ClangLinkerWrapper #20691
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[NewOffloadModel] Pass link-time options through device-compiler and device-linker argument for ClangLinkerWrapper #20691
Conversation
43ec9a6 to
2c3b7bc
Compare
|
The CI result of running with the new offloading model enabled as default with this patch can be found at https://github.com/intel/llvm/actions/runs/20794404398/job/59724084542?pr=20691. When comparing with the reference CI result (with only the new offloading model enabled as default, found at https://github.com/intel/llvm/actions/runs/20782646887/job/59683703668?pr=20570), we can see that 4 SYCL E2E tests (listed in the PR description) are now passing with the new offloading model and there are no regression tests. |
…we have comfirmed no regression
|
@intel/dpcpp-clang-driver-reviewers Could you please help review this PR? Thank you! |
| ArgStringList LinkerArgs; | ||
| const DerivedArgList &ToolChainArgs = | ||
| C.getArgsForToolChain(TC, /*BoundArch=*/"", Kind); | ||
| DerivedArgList BaseCompilerArgs(ToolChainArgs.getBaseArgs()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: When we invoke getBaseArgs() on getArgsForToolChain() , we get back the original argument list that existed before Clang tailored the arguments for that toolchain.
Why not just use Args here instead of creating a new redundant variable?
We could invoke : SYCLTC.AddImpliedTargetArgs(SYCLTC.getTriple(), Args, CompilerArgs, JA, *HostTC);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we can see in the following code, we are selectively populating BaseCompilerArgs:
for (Arg *A : ToolChainArgs) {
if (A->getOption().matches(OPT_Zlinker_input))
LinkerArgs.emplace_back(A->getValue());
else if (ShouldForward(CompilerOptions, A, *TC))
BaseCompilerArgs.append(A);
else if (ShouldForward(LinkerOptions, A, *TC))
A->render(Args, LinkerArgs);
}
DerivedArgList BaseCompilerArgs(ToolChainArgs.getBaseArgs()); creates a new container BaseCompilerArgs initialized with the base arguments, and we then update BaseCompilerArgs by selectively adding arguments from ToolChainArgs based on the filtering logic. Therefore, not every argument from Args or ToolChainArgs will be present in BaseCompilerArgs,. Therefore, I don't think we can directly use Args to replace BaseCompilerArgs. Please correct me if any mistake in my understanding. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BaseCompilerArgs is the original argument list. ToolChainArgs is a subset of BaseCompilerArgs - is that correct?
If so, how adding some options from ToolChainArgs to BaseCompilerArgs helps, when BaseCompilerArgs contains everything that ToolChainArgs contain already? Or what am I missing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@YixingZhang007 For this simple invocation:
clang++ -fsycl sycl.cpp
Args will be -fsycl sycl.cpp
ToolChainArgs will be (simplified) -fsycl sycl.cpp -fsycl-targets=spir64 -internal-isystem <sycl include path>
Calling getBaseArgs() on ToolChainArgs will return the original Args.
I was wondering if we could call append() on Args directly, but since Args is a const that is not possible.
So it makes sense to create BaseCompilerArgs to hold this new list.
One small nit is we could just do DerivedArgList BaseCompilerArgs(Args); but it nbd.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when BaseCompilerArgs contains everything that ToolChainArgs contain already?
I don't think BaseCompilerArgs will contain everything that ToolChainArgs contains.
Typically, getArgsForToolChain() produces a new argument list that keeps the original args but also adds all the required args needed for the selected toolchain.( additional SYCL args, include paths for SYCL headers etc that will not be in BaseCompilerArgs)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@srividya-sundaram Thank you for the explanation! In this case, I guess we would keep the code as it is then rn :)
|
I don't think the current CI failure is caused by changes in this PR. Before the last commit (which only modified a comment), all CI tests passed. @intel/llvm-gatekeepers could you please help merge this PR? Thank you! |
In another patch #19579, compilation and linking options used in ClangLinkerWrapper are changed to be consumed from the
SYCLImage, instead ofsycl_backend_compile_options_EQandsycl_target_link_options_EQthat are passed as argument to the ClangLinkerWrapper. Because option in SYCLImage are options passed at compiler-time, we want to support for link-time options by passing them through device-compiler and device-linker argument for ClangLinkerWrapper. The specific changes are listed below.gpu_tool_arg_EQandcpu_tool_arg_EQoptions specification inLinkerWrapperOpts.td. We are now passing the link time backend compile option throughOPT_device_compiler_args_EQto ClangLinkerWrapper, as coded inClang.cpp. We are also passing link time linker option throughOPT_device_linker_args_EQto ClangLinkerWrapper.Clang.cpp, we callAddImpliedTargetArgsto add SYCL target-specific arguments to ClangLinkerWrapper by processing the arguments passed to Clang.OPT_ftarget_register_alloc_mode_EQis added to theCompilerOptions so that it can be available inAddImpliedTargetArgsfor determining whether to add-ftarget-register-alloc-mode=xxxto the compiler arguments.ClangLinkerWrapper.cpp, the options inOPT_device_compiler_args_EQandOPT_device_linker_args_EQare added and handled together with other compiler and linker options passed throughSYCLImage.With this patch, the following SYCL E2E tests pass with the new offloading model: