-
Notifications
You must be signed in to change notification settings - Fork 794
[SYCL][clang-linker-wrapper] Fix argument parsing in Clang Linker Wrapper #20470
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
base: sycl
Are you sure you want to change the base?
Changes from 3 commits
edae614
6340ea0
c7bb8d5
1708433
263ce30
f8a1c0c
7d853a7
aba3bc0
f50124a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -1556,6 +1556,33 @@ static void appendOneArg(InputArgList &Args, const Arg *Opt) { | |||||
| } | ||||||
| } | ||||||
|
|
||||||
| /// Utility function to parse all devices passed via -fsycl-targets. | ||||||
| /// Return 'true' for JIT, AOT Intel CPU/GPUs and NVidia/AMD targets. | ||||||
| /// Otherwise return 'false'. | ||||||
| bool Driver::GetUseNewOffloadDriverForSYCLOffload(Compilation &C, | ||||||
| const ArgList &Args) const { | ||||||
| // Check only if enabled with -fsycl | ||||||
|
||||||
| // Check only if enabled with -fsycl | |
| // Check only if enabled with -fsycl. |
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.
Thank you for your suggestions! The changes for enabling the new offloading model in this PR were copied directly from another PR (#15121). This PR specifically focuses on resolving test failures in fp64-conv-emu-1.cpp and fp64-conv-emu-2.cpp that occur after enabling the new offloading model. I will move any comments related to the new offloading model changes to the original PR where that functionality was implemented. The changes related to enabling the new offloading model have been reverted and removed from this PR.
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.
Okay, I'm good with keeping it as is for this PR, but we can follow up on a subsequent PR to address this?
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.
For sure! I guess I will add these comments to PR #15121, which contains changes for enabling the new offloading model as the default. These changes will be applied when we reopen the PR after all SYCL E2E test issues have been resolved.
Outdated
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.
Support for FPGA related options have been removed in this PR.
Don't think this check is necessary.
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.
Thank you for your suggestions! The changes for enabling the new offloading model in this PR were copied directly from another PR (#15121). This PR specifically focuses on resolving test failures in fp64-conv-emu-1.cpp and fp64-conv-emu-2.cpp that occur after enabling the new offloading model. I will move any comments related to the new offloading model changes to the original PR where that functionality was implemented. The changes related to enabling the new offloading model have been reverted and removed from this PR.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -955,14 +955,20 @@ static void addBackendOptions(const ArgList &Args, | |
| // in any of them. | ||
| auto [BeforeOptions, AfterOptions] = OptC.split("-options "); | ||
| // Only add if not empty, an empty arg can lead to ocloc errors. | ||
| if (!BeforeOptions.empty()) | ||
| CmdArgs.push_back(BeforeOptions); | ||
| if (!BeforeOptions.empty()){ | ||
| SmallVector<StringRef, 8> BeforeArgs; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. also just wondering, i see one of your commits is titled says 'revert nick's PR' but i don't remember writing that code so if it is referring to me do you mind linking the PR being reverted, thanks There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Absolutely! I previously applied the changes directly from your PR #15121 (this PR is currently closed) to enable the new offloading model and verify the CI test results. After confirming that both There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe, resolving all SYCL E2E test failures with new offloading model is necessary to enable new model by default, but it is not the only condition. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would say we should add it to the nightly, but yeah IMO we can do it now if someone has bandwidth do to it. i definitely do not but im happy to help. We need the job to pass though, so we ideally would have a LIT variable so we can XFAIL the failing ones, or more crudely we coukd have some file with a list of either passing or failing tests, in that case we could use LIT_FILTER or LIT_FILTER_OUT btw that pr isn't from me but doesnt matter either way :P There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I will ask someone to add it to nightly. I think we should do it similar to how spirv_backend is being tested, likely with a LIT variable. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1. |
||
| BeforeOptions.split(BeforeArgs, " ", /*MaxSplit=*/-1, /*KeepEmpty=*/false); | ||
| for (auto string : BeforeArgs) { | ||
YixingZhang007 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| CmdArgs.push_back(string); | ||
| } | ||
| } | ||
| if (!AfterOptions.empty()) { | ||
| // Separator not included by the split function, so explicitly added here. | ||
| CmdArgs.push_back("-options"); | ||
| std::string Replace = AfterOptions.str(); | ||
| std::replace(Replace.begin(), Replace.end(), ' ', ','); | ||
| CmdArgs.push_back(Args.MakeArgString(Replace)); | ||
| SmallVector<StringRef, 8> AfterArgs; | ||
| AfterOptions.split(AfterArgs, " ", /*MaxSplit=*/-1, /*KeepEmpty=*/false); | ||
| std::string JoinedOptions = llvm::join(AfterArgs, " "); | ||
YixingZhang007 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| CmdArgs.push_back(Args.MakeArgString(JoinedOptions)); | ||
| } | ||
| } | ||
| StringRef OptL = | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,6 +20,9 @@ | |
| // RUN: %clangxx -fsycl -fsycl-targets=intel_gpu_dg2_g10,intel_gpu_dg2_g11,intel_gpu_dg2_g12,intel_gpu_pvc,intel_gpu_mtl_h,intel_gpu_mtl_u -fsycl-fp64-conv-emu %O0 %s -o %t.out | ||
| // RUN: %{run} %t.out | ||
|
|
||
| // RUN: %clangxx -fsycl -fsycl-targets=intel_gpu_dg2_g10,intel_gpu_dg2_g11,intel_gpu_dg2_g12,intel_gpu_pvc,intel_gpu_mtl_h,intel_gpu_mtl_u -fsycl-fp64-conv-emu --offload-new-driver %O0 %s -o %t.out | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we also try with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The changes work with '-g' and I have also added a new test command in this file for running the test with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. continue my previous comment: we can keep -g test, but just removing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you for your suggestion!
Do you mean that we should replace the third case to be "old offloading model and -g"? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Original test was testing 1 case:
I suggest to add one more case, so result will be:
We should not distinguish between new and old offloading model in this test. Instead, we should just add new entire SYCL E2E testing with new offloading model enabled by option for all tests and of course it should be separate PR. If you still have questions, feel free to ping me in teams and we can discuss. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Following up on this: there is a specific directory for New Offloading Model tests: https://github.com/intel/llvm/blob/sycl/sycl/test-e2e/NewOffloadDriver/ Everything related to the New Offloading Model should go there, I think. Also, I agree that this can be done in a separate PR. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My point was that instead of adding individual tests for new offloading model duplicating old offloading model tests, we should just start running entire SYCL E2E with new offloading model enabled. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right, but while we do that, if we need to add individual tests, they should go to the specific directory. Once we start running entire SYCL E2E with new offloading model, we can drop the specific directory, I think. |
||
| // RUN: %{run} %t.out | ||
|
|
||
| // Tests that aspect::fp64 is not emitted correctly when -fsycl-fp64-conv-emu | ||
| // flag is used. | ||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.