[SYCL] Call clang instead of llc. #15259

hvdijk · 2024-09-02T22:53:37Z

Previously, OffloadWrapper::ConstructJob would call llc to convert LLVM IR into machine code. With this change, it calls clang instead.

This should be mostly NFC for x86, there may be some small changes in exactly which passes run, but they are intended to be roughly the same, and enables future changes passing along more clang options.

For other targets, this is a bugfix. Specifically, for RISC-V, llc and clang disagree on which subtarget features to enable by default, resulting in linker errors due to subtarget feature mismatches between the llc-compiled wrapper and other clang-compiled object files.

Previously, OffloadWrapper::ConstructJob would call llc to convert LLVM IR into machine code. With this change, it calls clang instead. This should be mostly NFC for x86, there may be some small changes in exactly which passes run, but they are intended to be roughly the same, and enables future changes passing along more clang options. For other targets, this is a bugfix. Specifically, for RISC-V, llc and clang disagree on which subtarget features to enable by default, resulting in linker errors due to subtarget feature mismatches between the llc-compiled wrapper and other clang-compiled object files.

* Windows does not support, and does not need, -fPIC. * Update lit tests.

mdtoguchi · 2024-09-03T14:37:35Z

clang/lib/Driver/ToolChains/Clang.cpp

+      const char *Clang = C.getArgs().MakeArgString(ClangPath);
+      C.addCommand(std::make_unique<Command>(JA, *this,
+                                             ResponseFileSupport::None(), Clang,
+                                             ClangArgs, std::nullopt));


I think ideally, this should be handled outside of the wrapper call, where we call the wrapper, then create a new job that calls the compiler backend to create the object. But as we are moving away from the old offload model in the near future that work may be wasted effort.

I agree it should not be in this function directly, though I had a slightly different idea there, that is what the // TODO Use TC.SelectTool(). comment is about. I was concerned it would be too big a change to go that route right away, and I would have that same concern with your idea. But if you say this whole approach is going away anyway, then definitely it sounds like it is not worth the extra work.

mdtoguchi · 2024-09-03T14:45:31Z

clang/lib/Driver/ToolChains/Clang.cpp

      llvm::Reloc::Model RelocationModel;
      unsigned PICLevel;
      bool IsPIE;
      std::tie(RelocationModel, PICLevel, IsPIE) =
          ParsePICArgs(getToolChain(), TCArgs);
      if (PICLevel > 0 || TCArgs.hasArg(options::OPT_shared)) {
-        LlcArgs.push_back("-relocation-model=pic");
+        if (!TC.getAuxTriple()->isOSWindows())


Can you explain why we need this restriction?

Windows does not support (or need) -fPIC, it results in "clang: error: unsupported option '-fPIC' for target 'x86_64-pc-windows-msvc'".

hvdijk · 2024-09-16T12:25:18Z

ping

mdtoguchi

Thanks - LGTM.

bader · 2024-09-16T15:31:27Z

@hvdijk, @mdtoguchi, is llc used for anything else in the SYCL compilation flow?
If no, please, remove it from the deploy-sycl-toolchain target.
https://github.com/intel/llvm/blob/sycl/sycl/CMakeLists.txt#L443

maksimsab · 2024-09-16T15:34:51Z

@bader Yes, it is being used in the clang-linker-wrapper at the moment.

llvm/clang/test/Driver/linker-wrapper-sycl.cpp

Line 36 in db75b03

    
           // CHK-SPLIT-CMDS-NEXT: "{{.*}}llc" -filetype=obj -o [[LLCOUT:.*]] [[WRAPPEROUT]].bc

hvdijk · 2024-09-16T15:39:11Z

@bader Yes, it is being used in the clang-linker-wrapper at the moment.

Looking at that, probably that would benefit from the same change; if so, would you prefer that I update this PR to do so or would you prefer that as a later followup PR?

bader · 2024-09-16T15:58:44Z

@bader Yes, it is being used in the clang-linker-wrapper at the moment.

Looking at that, probably that would benefit from the same change; if so, would you prefer that I update this PR to do so or would you prefer that as a later followup PR?

Eventually, we will switch to the new offloading model, so I would do the change sooner rather than later, but I don't have a strong opinion about the form. I leave this decision to you.
If you split the change into PRs, I suggest updating PR title or description to say that we update only "old" offload model.

- Update clang-offload-wrapper too. - Remove unused SYCL::Linker::constructLlcCommand.

hvdijk · 2024-09-17T16:14:34Z

Updated to modify clang-linker-wrapper too (with an incorrect name in the commit message, but that is going to be squashed anyway) and to remove SYCL::Linker::constructLlcCommand.

I did not modify deploy-sycl-toolchain yet because I noticed that llc is still called when compiling SPIR-V files (see clang/lib/Driver/ToolChains/Clang.cpp) and I was not sure whether this is a scenario that is important to Intel.

clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp

asudarsa · 2024-09-17T17:05:30Z

Updated to modify clang-linker-wrapper too (with an incorrect name in the commit message, but that is going to be squashed anyway) and to remove SYCL::Linker::constructLlcCommand.

I did not modify deploy-sycl-toolchain yet because I noticed that llc is still called when compiling SPIR-V files (see clang/lib/Driver/ToolChains/Clang.cpp) and I was not sure whether this is a scenario that is important to Intel.

Hi @hvdijk

Thanks so much for updating the clang-linker-wrapper. w.r.t the use of llc for SPIR-V code generation, it is a temporary workaround I added to test the SPIR-V backend. I will be removing that work-around soon and will remove the llc dependency for sycl toolchain at that point.
Hope that's agreeable.

Thanks

asudarsa

LGTM. Thanks

PietroGhg · 2024-09-18T11:15:10Z

@intel/llvm-gatekeepers this looks ready to merge, thank you

hvdijk requested a review from a team as a code owner September 2, 2024 22:53

hvdijk had a problem deploying to WindowsCILock September 3, 2024 01:13 — with GitHub Actions Failure

hvdijk had a problem deploying to WindowsCILock September 3, 2024 01:45 — with GitHub Actions Failure

Fixes.

756e9ed

* Windows does not support, and does not need, -fPIC. * Update lit tests.

hvdijk temporarily deployed to WindowsCILock September 3, 2024 13:23 — with GitHub Actions Inactive

hvdijk temporarily deployed to WindowsCILock September 3, 2024 13:56 — with GitHub Actions Inactive

mdtoguchi reviewed Sep 3, 2024

View reviewed changes

mdtoguchi approved these changes Sep 16, 2024

View reviewed changes

More updates:

63ed85b

- Update clang-offload-wrapper too. - Remove unused SYCL::Linker::constructLlcCommand.

hvdijk requested a review from a team as a code owner September 17, 2024 16:09

hvdijk had a problem deploying to WindowsCILock September 17, 2024 16:12 — with GitHub Actions Error

bader approved these changes Sep 17, 2024

View reviewed changes

clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp Show resolved Hide resolved

Update a few more comments to remove references to llc.

0b1b881

hvdijk had a problem deploying to WindowsCILock September 17, 2024 16:33 — with GitHub Actions Failure

asudarsa approved these changes Sep 17, 2024

View reviewed changes

hvdijk temporarily deployed to WindowsCILock September 17, 2024 18:17 — with GitHub Actions Inactive

Update the Windows version of the test too.

2c6680e

hvdijk temporarily deployed to WindowsCILock September 18, 2024 09:25 — with GitHub Actions Inactive

hvdijk temporarily deployed to WindowsCILock September 18, 2024 10:02 — with GitHub Actions Inactive

martygrant merged commit 464b077 into intel:sycl Sep 18, 2024
11 checks passed

hvdijk deleted the clang-instead-of-llc branch September 18, 2024 12:19

hvdijk mentioned this pull request Apr 8, 2025

intel-llvm-sycl/dpcpp compiler linking error: soft-float modules with double-float modules #17726

Closed

jzc mentioned this pull request Apr 8, 2025

[Driver] Wrapper compilation drops target features #14139

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[SYCL] Call clang instead of llc. #15259

[SYCL] Call clang instead of llc. #15259

Uh oh!

hvdijk commented Sep 2, 2024

Uh oh!

mdtoguchi Sep 3, 2024

Uh oh!

hvdijk Sep 3, 2024

Uh oh!

mdtoguchi Sep 3, 2024

Uh oh!

hvdijk Sep 3, 2024

Uh oh!

hvdijk commented Sep 16, 2024

Uh oh!

mdtoguchi left a comment

Uh oh!

bader commented Sep 16, 2024

Uh oh!

maksimsab commented Sep 16, 2024

Uh oh!

hvdijk commented Sep 16, 2024

Uh oh!

bader commented Sep 16, 2024

Uh oh!

hvdijk commented Sep 17, 2024

Uh oh!

Uh oh!

asudarsa commented Sep 17, 2024

Uh oh!

asudarsa left a comment

Uh oh!

PietroGhg commented Sep 18, 2024

Uh oh!

Uh oh!

Uh oh!

[SYCL] Call clang instead of llc. #15259

[SYCL] Call clang instead of llc. #15259

Uh oh!

Conversation

hvdijk commented Sep 2, 2024

Uh oh!

mdtoguchi Sep 3, 2024

Choose a reason for hiding this comment

Uh oh!

hvdijk Sep 3, 2024

Choose a reason for hiding this comment

Uh oh!

mdtoguchi Sep 3, 2024

Choose a reason for hiding this comment

Uh oh!

hvdijk Sep 3, 2024

Choose a reason for hiding this comment

Uh oh!

hvdijk commented Sep 16, 2024

Uh oh!

mdtoguchi left a comment

Choose a reason for hiding this comment

Uh oh!

bader commented Sep 16, 2024

Uh oh!

maksimsab commented Sep 16, 2024

Uh oh!

hvdijk commented Sep 16, 2024

Uh oh!

bader commented Sep 16, 2024

Uh oh!

hvdijk commented Sep 17, 2024

Uh oh!

Uh oh!

asudarsa commented Sep 17, 2024

Uh oh!

asudarsa left a comment

Choose a reason for hiding this comment

Uh oh!

PietroGhg commented Sep 18, 2024

Uh oh!

Uh oh!

Uh oh!