Add clang-offload-deps tool for generating dependence files for offload targets #2872

sndmitriev · 2020-12-08T06:34:27Z

This tool is intended to be used by the clang driver for offload linking with
static offload libraries. It takes linked host image as input and produces
bitcode files (one per offload target) containing references to symbols that
must be defined in the target images. Each dependence bitcode file then is
expected to be compiled to an object by the driver using appropriate target
toolchain, and dependence object added to the target linker as input together
with the other inputs. References to the symbols in dependence object should
ensure that target linker pulls in necessary symbol definitions from the input
static libraries.

Signed-off-by: Sergey Dmitriev serguei.n.dmitriev@intel.com

…ad targets This tool is intended to be used by the clang driver for offload linking with static offload libraries. It takes linked host image as input and produces bitcode files (one per offload target) containing references to symbols that must be defined in the target images. Each dependence bitcode file then is expected to be compiled to an object by the driver using appropriate target toolchain, and dependence object added to the target linker as input together with the other inputs. References to the symbols in dependence object should ensure that target linker pulls in necessary symbol definitions from the input static libraries. Signed-off-by: Sergey Dmitriev <serguei.n.dmitriev@intel.com>

kbobrovs · 2020-12-08T12:06:09Z

clang/tools/clang-offload-deps/ClangOffloadDeps.cpp

+static cl::OptionCategory
+    ClangOffloadDepsCategory("clang-offload-deps options");
+
+static cl::list<std::string> Outputs("outputs", cl::CommaSeparated,


Usually multiple outputs within the driver are handled either via file lists (TY_Tempfilelist) or file tables (TY_Tempfiletable). Driver implementation would easier and more inline with other multi-output tools if this tool could generate a file list listing all the outputs separated by \n.
@mdtoguchi - what do you think?

This interface is consistent with other clang-offload tools.

I would expect that the outputs match the targets, which is in-line with the bundler. I'm OK with how things are.

clang/tools/clang-offload-deps/ClangOffloadDeps.cpp

kbobrovs · 2020-12-08T12:22:14Z

clang/tools/clang-offload-deps/ClangOffloadDeps.cpp

+            ConstantArray::get(ArrayTy, Used), "llvm.used");
+        GV->setSection("llvm.metadata");
+      } else {
+        auto *GV = new GlobalVariable(


I think from code maintenance perspective it is better to have single mechanism for SPIRV and non-SPIRV. Will the second work for SPIRV target as well? If yes, can the first one be discarded?

Well, LLVM IR just allows to create a reference without adding any additional symbols, I do not see why we should not use this feature. I would rather discard the second part if we had a way to add a reference without adding extra symbols, but looks like this is the only way to do it.

AFAIU, both achieve the same result - letting linker know the deps - and only the second can handle all targets. Hence my suggestion. This is a nit anyway.

bader · 2020-12-08T14:32:42Z

@AlexeySachkov, FYI.

bader · 2020-12-08T14:35:26Z

@sndmitriev, please, add yourself as a code owner for this tool to https://github.com/intel/llvm/blob/sycl/.github/CODEOWNERS.

Signed-off-by: Sergey Dmitriev <serguei.n.dmitriev@intel.com>

clang/test/Driver/clang-offload-deps.c

bader · 2020-12-08T14:39:55Z

sycl/CMakeLists.txt

@@ -280,6 +280,7 @@ add_custom_target( sycl-toolchain
          clang
          clang-offload-wrapper
          clang-offload-bundler
+          clang-offload-deps


I think we should add this tool to sycl-toolchain, but if I understand it correctly, the tool is not integrated into compilation flow yet.
I suggest we add it here once the driver make use of it.

Yes, this tool is not integrated into the compilation flow yet.

clang/tools/clang-offload-deps/ClangOffloadDeps.cpp

mdtoguchi · 2020-12-08T16:55:12Z

clang/tools/clang-offload-deps/ClangOffloadDeps.cpp

+static cl::OptionCategory
+    ClangOffloadDepsCategory("clang-offload-deps options");
+
+static cl::list<std::string> Outputs("outputs", cl::CommaSeparated,


I would expect that the outputs match the targets, which is in-line with the bundler. I'm OK with how things are.

clang/tools/clang-offload-deps/ClangOffloadDeps.cpp

Co-authored-by: mdtoguchi <47896532+mdtoguchi@users.noreply.github.com>

sndmitriev · 2020-12-09T04:25:13Z

@sndmitriev, please, add yourself as a code owner for this tool to https://github.com/intel/llvm/blob/sycl/.github/CODEOWNERS.

Done

bader

Thanks!

sndmitriev · 2020-12-17T02:43:51Z

Does this patch need any additional changes to be merged?

romanovvlad · 2020-12-17T11:28:12Z

@sndmitriev There is merge conflict in .github/CODEOWNERS, could you please resolve it?

sndmitriev · 2020-12-17T13:08:35Z

@sndmitriev There is merge conflict in .github/CODEOWNERS, could you please resolve it?

Done.

sndmitriev · 2020-12-20T04:22:05Z

I have resolved conflict. Does this change need anything else to be merged?

romanovvlad · 2020-12-21T10:42:23Z

I have resolved conflict. Does this change need anything else to be merged?

Github UI says:
Waiting on code owner review from intel/llvm-reviewers-runtime, AGindinson, mdtoguchi, elizabethandrews, Fznamznon, and/or premanandrao.

mdtoguchi

LGTM

elizabethandrews

The patch looks ok to me but I am unfamiliar with this and so I don’t think I’m the right person to review this. @kbobrovs @bader could you please take a look?

sndmitriev requested review from bader, kbobrovs and mdtoguchi December 8, 2020 06:34

sndmitriev requested review from AGindinson, elizabethandrews, Fznamznon, premanandrao and a team as code owners December 8, 2020 06:34

sndmitriev requested a review from romanovvlad December 8, 2020 06:34

kbobrovs reviewed Dec 8, 2020

View reviewed changes

Addressed review comments.

007bcac

Signed-off-by: Sergey Dmitriev <serguei.n.dmitriev@intel.com>

bader reviewed Dec 8, 2020

View reviewed changes

Updated LIT test to address review comments.

9652971

mdtoguchi reviewed Dec 8, 2020

View reviewed changes

Apply suggestions from code review

293f6a0

Co-authored-by: mdtoguchi <47896532+mdtoguchi@users.noreply.github.com>

bader previously approved these changes Dec 9, 2020

View reviewed changes

kbobrovs previously approved these changes Dec 10, 2020

View reviewed changes

romanovvlad previously approved these changes Dec 10, 2020

View reviewed changes

Merge branch 'sycl' into public/sndmitriev/clang-offload-deps

90868d3

sndmitriev dismissed stale reviews from romanovvlad, kbobrovs, and bader via 90868d3 December 17, 2020 13:06

bader approved these changes Dec 18, 2020

View reviewed changes

romanovvlad approved these changes Dec 21, 2020

View reviewed changes

mdtoguchi approved these changes Dec 21, 2020

View reviewed changes

elizabethandrews reviewed Dec 21, 2020

View reviewed changes

pvchupin merged commit 94d2158 into intel:sycl Dec 21, 2020

sndmitriev deleted the public/sndmitriev/clang-offload-deps branch January 4, 2021 04:01

sndmitriev mentioned this pull request Jan 11, 2021

[SYCL] Do not emit "llvm.used" to output module(s) in the post link tool #2995

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add clang-offload-deps tool for generating dependence files for offload targets #2872

Add clang-offload-deps tool for generating dependence files for offload targets #2872

sndmitriev commented Dec 8, 2020

kbobrovs Dec 8, 2020

sndmitriev Dec 8, 2020

mdtoguchi Dec 8, 2020

kbobrovs Dec 8, 2020

sndmitriev Dec 8, 2020

kbobrovs Dec 9, 2020

bader commented Dec 8, 2020

bader commented Dec 8, 2020

bader Dec 8, 2020

sndmitriev Dec 9, 2020

mdtoguchi Dec 8, 2020

sndmitriev commented Dec 9, 2020 •

edited

Loading

bader left a comment

sndmitriev commented Dec 17, 2020

romanovvlad commented Dec 17, 2020

sndmitriev commented Dec 17, 2020

sndmitriev commented Dec 20, 2020

romanovvlad commented Dec 21, 2020

mdtoguchi left a comment

elizabethandrews left a comment •

edited

Loading

Add clang-offload-deps tool for generating dependence files for offload targets #2872

Add clang-offload-deps tool for generating dependence files for offload targets #2872

Conversation

sndmitriev commented Dec 8, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bader commented Dec 8, 2020

bader commented Dec 8, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sndmitriev commented Dec 9, 2020 • edited Loading

bader left a comment

Choose a reason for hiding this comment

sndmitriev commented Dec 17, 2020

romanovvlad commented Dec 17, 2020

sndmitriev commented Dec 17, 2020

sndmitriev commented Dec 20, 2020

romanovvlad commented Dec 21, 2020

mdtoguchi left a comment

Choose a reason for hiding this comment

elizabethandrews left a comment • edited Loading

Choose a reason for hiding this comment

sndmitriev commented Dec 9, 2020 •

edited

Loading

elizabethandrews left a comment •

edited

Loading