[PATCH] D64943: [Clang][OpenMP offload] Eliminate use of OpenMP linker script

sdmitriev abandoned this revision. sdmitriev added a comment. All three parts have been committed, so I am abandoning the original patch. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64943/new/ https://reviews.llvm.org/D64943 ___ cfe-commi

[PATCH] D64943: [Clang][OpenMP offload] Eliminate use of OpenMP linker script

sdmitriev added a comment. I have uploaded the last part to https://reviews.llvm.org/D68746 CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64943/new/ https://reviews.llvm.org/D64943 ___ cfe-commits mailing list cfe-commits@lists.llvm.org htt

[PATCH] D64943: [Clang][OpenMP offload] Eliminate use of OpenMP linker script

sdmitriev added a comment. The second part was uploaded to https://reviews.llvm.org/D68166. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64943/new/ https://reviews.llvm.org/D64943 ___ cfe-commits mailing list cfe-commits@lists.llvm.org htt

[PATCH] D64943: [Clang][OpenMP offload] Eliminate use of OpenMP linker script

sdmitriev added a comment. I have uploaded the first part to https://reviews.llvm.org/D68070 CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64943/new/ https://reviews.llvm.org/D64943 ___ cfe-commits mailing list cfe-commits@lists.llvm.org ht

[PATCH] D64943: [Clang][OpenMP offload] Eliminate use of OpenMP linker script

hfinkel added a comment. In D64943#1683368 , @JonChesterfield wrote: > The three way split looks great, thanks. Makes sense to me. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64943/new/ https://reviews.llvm.org/D64943 ___

[PATCH] D64943: [Clang][OpenMP offload] Eliminate use of OpenMP linker script

JonChesterfield added a comment. The three way split looks great, thanks. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64943/new/ https://reviews.llvm.org/D64943 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.or

[PATCH] D64943: [Clang][OpenMP offload] Eliminate use of OpenMP linker script

ABataev added inline comments. Comment at: clang/lib/Driver/Driver.cpp:3125 +ActionList DeviceAL; +for (auto *SB : SpecializedBuilders) { + if (!SB->isValid()) User real type instead of `auto *` Comment at: clang/lib/Driver/Driver.

[PATCH] D64943: [Clang][OpenMP offload] Eliminate use of OpenMP linker script

JonChesterfield added a comment. In D64943#1682452 , @hfinkel wrote: > This LGTM. I'm happy that this is a design improvement over the current > scheme. @JonChesterfield , @ABataev , any further comments? This patch mixes two concerns. 1/ Remove the li

[PATCH] D64943: [Clang][OpenMP offload] Eliminate use of OpenMP linker script

hfinkel added a comment. This LGTM. I'm happy that this is a design improvement over the current scheme. @JonChesterfield , @ABataev , any further comments? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64943/new/ https://reviews.llvm.org/D64943 _

[PATCH] D64943: [Clang][OpenMP offload] Eliminate use of OpenMP linker script

JonChesterfield added a comment. > I see some problems with using llvm-objcopy for that. First issue is that > symbols created by llvm-objcopy for embedded data depend on the input file > name. As you know these symbols are referenced from the offload registration > code that is currently added

[PATCH] D64943: [Clang][OpenMP offload] Eliminate use of OpenMP linker script

sdmitriev added a comment. In D64943#1668006 , @JonChesterfield wrote: > >> Does this diff mix getting rid of the linker script with other changes? > >> E.g. it looks like the metadata generation is moving from clang to the new > >> tool, but that seems

[PATCH] D64943: [Clang][OpenMP offload] Eliminate use of OpenMP linker script

JonChesterfield added a comment. >> Does this diff mix getting rid of the linker script with other changes? E.g. >> it looks like the metadata generation is moving from clang to the new tool, >> but that seems orthogonal to dropping the linker script. > > Metadata is still generated by the clan

[PATCH] D64943: [Clang][OpenMP offload] Eliminate use of OpenMP linker script

sdmitriev added a comment. In D64943#1667469 , @JonChesterfield wrote: > I'm on board with getting rid of the linker script. Gold's limited support > for that seems conclusive. > > I believe the current script does two things: > 1/ takes a binary and em

[PATCH] D64943: [Clang][OpenMP offload] Eliminate use of OpenMP linker script

JonChesterfield added a comment. I'm on board with getting rid of the linker script. Gold's limited support for that seems conclusive. I believe the current script does two things: 1/ takes a binary and embeds it in a section named .omp_offloading.amdgcn-amd-amdhsa 2/ provides start/end symbols

[PATCH] D64943: [Clang][OpenMP offload] Eliminate use of OpenMP linker script

JonChesterfield added a comment. > Hm, I was not aware of this Linux linker feature, thanks a lot for the > explanation! I see only one problem with using it as a replacement for the > begin/end objects – it looks like `__start_name`/`__stop_name` symbols are > created with `default` visibility

[PATCH] D64943: [Clang][OpenMP offload] Eliminate use of OpenMP linker script

sdmitriev added a comment. In D64943#1666924 , @JonChesterfield wrote: > In D64943#1666849 , @sdmitriev wrote: > > > In D64943#136 , > > @JonChesterfield wrote: > > > >

[PATCH] D64943: [Clang][OpenMP offload] Eliminate use of OpenMP linker script

JonChesterfield added a comment. In D64943#1666849 , @sdmitriev wrote: > In D64943#136 , @JonChesterfield > wrote: > > > I'm not sure copying the crtbegin/crtend mechanism from the early days of C > > runtime

[PATCH] D64943: [Clang][OpenMP offload] Eliminate use of OpenMP linker script

sdmitriev added a comment. In D64943#136 , @JonChesterfield wrote: > I'm not sure copying the crtbegin/crtend mechanism from the early days of C > runtime is ideal. Since the data is stored in a common section anyway, please > could we rename it to

RE: [PATCH] D64943: [Clang][OpenMP offload] Eliminate use of OpenMP linker script

-commits@lists.llvm.org; mgo...@gentoo.org; zhang.guans...@gmail.com; mgrang.1...@gmail.com; t...@google.com; ztur...@roblox.com; peter.wal...@arm.com Subject: [PATCH] D64943: [Clang][OpenMP offload] Eliminate use of OpenMP linker script JonChesterfield added a comment. In D64943#173 <ht

[PATCH] D64943: [Clang][OpenMP offload] Eliminate use of OpenMP linker script

sdmitriev added a comment. In D64943#193 , @JonChesterfield wrote: > In D64943#179 , @ABataev wrote: > > > In D64943#178 , > > @JonChesterfield wrote: > > > > >

[PATCH] D64943: [Clang][OpenMP offload] Eliminate use of OpenMP linker script

JonChesterfield added a comment. In D64943#179 , @ABataev wrote: > In D64943#178 , @JonChesterfield > wrote: > > > In D64943#173 , @ABataev wrote: > > > > > In D

[PATCH] D64943: [Clang][OpenMP offload] Eliminate use of OpenMP linker script

ABataev added a comment. In D64943#178 , @JonChesterfield wrote: > In D64943#173 , @ABataev wrote: > > > In D64943#158 , > > @JonChesterfield wrote: > > > > > >

[PATCH] D64943: [Clang][OpenMP offload] Eliminate use of OpenMP linker script

JonChesterfield added a comment. In D64943#173 , @ABataev wrote: > In D64943#158 , @JonChesterfield > wrote: > > > > OpenMP linker script is known to cause problems for gold and lld linkers > > > on Linux

[PATCH] D64943: [Clang][OpenMP offload] Eliminate use of OpenMP linker script

ABataev added a comment. In D64943#158 , @JonChesterfield wrote: > > OpenMP linker script is known to cause problems for gold and lld linkers on > > Linux and it will also cause problems for Windows enabling in future > > What are the known problems

[PATCH] D64943: [Clang][OpenMP offload] Eliminate use of OpenMP linker script

JonChesterfield added a comment. > OpenMP linker script is known to cause problems for gold and lld linkers on > Linux and it will also cause problems for Windows enabling in future What are the known problems with the linker script? I'm wondering if they can be resolved without the overhead of

[PATCH] D64943: [Clang][OpenMP offload] Eliminate use of OpenMP linker script

JonChesterfield added a comment. I'm not sure copying the crtbegin/crtend mechanism from the early days of C runtime is ideal. Since the data is stored in a common section anyway, please could we rename it to __omp_offloading_entries in which case the linker will provide start/end symbols autom

[PATCH] D64943: [Clang][OpenMP offload] Eliminate use of OpenMP linker script

sdmitriev added a comment. Ping CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64943/new/ https://reviews.llvm.org/D64943 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D64943: [Clang][OpenMP offload] Eliminate use of OpenMP linker script

sdmitriev added a comment. Looks like there will be no more comments. If so, I will update the first part https://reviews.llvm.org/D65130 which adds clang-offload-wrapper tool with the latest changes. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64943/new/ https://reviews.llvm.org/D6

[PATCH] D64943: [Clang][OpenMP offload] Eliminate use of OpenMP linker script

sdmitriev added a comment. Ping. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64943/new/ https://reviews.llvm.org/D64943 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D64943: [Clang][OpenMP offload] Eliminate use of OpenMP linker script

sdmitriev added a comment. In D64943#1633164 , @ABataev wrote: > In D64943#1619958 , @sdmitriev wrote: > > > As I understand ‘atexit’ solution would be target dependent (‘__cxa_atexit’ > > on Linux and ‘atexit’ on

[PATCH] D64943: [Clang][OpenMP offload] Eliminate use of OpenMP linker script

sdmitriev marked 2 inline comments as done. sdmitriev added inline comments. Comment at: clang/tools/clang-offload-wrapper/ClangOffloadWrapper.cpp:72 +private: + IntegerType *getSizeTTy() { +switch (M.getDataLayout().getPointerTypeSize(Type::getInt8PtrTy(C))) { -

[PATCH] D64943: [Clang][OpenMP offload] Eliminate use of OpenMP linker script

vzakhari added a comment. FYI, llvm.global_dtor fix is in https://reviews.llvm.org/D66373 CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64943/new/ https://reviews.llvm.org/D64943 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https

[PATCH] D64943: [Clang][OpenMP offload] Eliminate use of OpenMP linker script

vzakhari added a comment. In D64943#1633372 , @lebedev.ri wrote: > In D64943#1633357 , @vzakhari wrote: > > > In D64943#1633175 , @ABataev wrote: > > > > > In D64943#1633170

[PATCH] D64943: [Clang][OpenMP offload] Eliminate use of OpenMP linker script

lebedev.ri added a comment. In D64943#1633357 , @vzakhari wrote: > In D64943#1633175 , @ABataev wrote: > > > In D64943#1633170 , @lebedev.ri > > wrote: > > > > > In D64943#1

[PATCH] D64943: [Clang][OpenMP offload] Eliminate use of OpenMP linker script

vzakhari added a comment. In D64943#1633175 , @ABataev wrote: > In D64943#1633170 , @lebedev.ri > wrote: > > > In D64943#1633164 , @ABataev wrote: > > > > > In D64943#161995

[PATCH] D64943: [Clang][OpenMP offload] Eliminate use of OpenMP linker script

ABataev added a comment. In D64943#1633170 , @lebedev.ri wrote: > In D64943#1633164 , @ABataev wrote: > > > In D64943#1619958 , @sdmitriev > > wrote: > > > > > As I understa

[PATCH] D64943: [Clang][OpenMP offload] Eliminate use of OpenMP linker script

lebedev.ri added a comment. In D64943#1633164 , @ABataev wrote: > In D64943#1619958 , @sdmitriev wrote: > > > As I understand ‘atexit’ solution would be target dependent (‘__cxa_atexit’ > > on Linux and ‘atexit’ on

[PATCH] D64943: [Clang][OpenMP offload] Eliminate use of OpenMP linker script

ABataev added a comment. In D64943#1619958 , @sdmitriev wrote: > As I understand ‘atexit’ solution would be target dependent (‘__cxa_atexit’ > on Linux and ‘atexit’ on Windows) whereas @llvm.global_ctors/dtors variables > offer similar and platform neutr

[PATCH] D64943: [Clang][OpenMP offload] Eliminate use of OpenMP linker script

sdmitriev added a comment. As I understand ‘atexit’ solution would be target dependent (‘__cxa_atexit’ on Linux and ‘atexit’ on Windows) whereas @llvm.global_ctors/dtors variables offer similar and platform neutral functionality (http://llvm.org/docs/LangRef.html#the-llvm-global-ctors-global-va

[PATCH] D64943: [Clang][OpenMP offload] Eliminate use of OpenMP linker script

vzakhari added inline comments. Comment at: clang/tools/clang-offload-wrapper/ClangOffloadWrapper.cpp:226 +// Add this function to global destructors. +appendToGlobalDtors(M, Func, 0); + } ABataev wrote: > sdmitriev wrote: > > vzakhari wrote: > > > FYI,

[PATCH] D64943: [Clang][OpenMP offload] Eliminate use of OpenMP linker script

ABataev added inline comments. Comment at: clang/tools/clang-offload-wrapper/ClangOffloadWrapper.cpp:226 +// Add this function to global destructors. +appendToGlobalDtors(M, Func, 0); + } sdmitriev wrote: > vzakhari wrote: > > FYI, llvm.global_dtors does

[PATCH] D64943: [Clang][OpenMP offload] Eliminate use of OpenMP linker script

sdmitriev marked 3 inline comments as done. sdmitriev added inline comments. Comment at: clang/tools/clang-offload-wrapper/ClangOffloadWrapper.cpp:226 +// Add this function to global destructors. +appendToGlobalDtors(M, Func, 0); + } vzakhari wrote: > FY

[PATCH] D64943: [Clang][OpenMP offload] Eliminate use of OpenMP linker script

vzakhari added inline comments. Comment at: clang/tools/clang-offload-wrapper/ClangOffloadWrapper.cpp:226 +// Add this function to global destructors. +appendToGlobalDtors(M, Func, 0); + } FYI, llvm.global_dtors does not work on Windows. The symbol will

[PATCH] D64943: [Clang][OpenMP offload] Eliminate use of OpenMP linker script

sdmitriev marked 14 inline comments as done. sdmitriev added inline comments. Comment at: clang/tools/clang-offload-wrapper/ClangOffloadWrapper.cpp:69 + + using MemoryBuffersVector = SmallVectorImpl>; + ABataev wrote: > Why not `ArrayRef`? Changed to MemoryBuffe

[PATCH] D64943: [Clang][OpenMP offload] Eliminate use of OpenMP linker script

ABataev added inline comments. Comment at: clang/tools/clang-offload-wrapper/ClangOffloadWrapper.cpp:69 + + using MemoryBuffersVector = SmallVectorImpl>; + Why not `ArrayRef`? Comment at: clang/tools/clang-offload-wrapper/ClangOffloadWrapper.c

[PATCH] D64943: [Clang][OpenMP offload] Eliminate use of OpenMP linker script

sdmitriev marked 4 inline comments as done. sdmitriev added inline comments. Comment at: clang/include/clang/Driver/Action.h:74 OffloadUnbundlingJobClass, +OffloadWrapperJobClass, ABataev wrote: > Do we really need this new kind of job here, can we use

[PATCH] D64943: [Clang][OpenMP offload] Eliminate use of OpenMP linker script

ABataev added inline comments. Comment at: clang/include/clang/Driver/Action.h:74 OffloadUnbundlingJobClass, +OffloadWrapperJobClass, Do we really need this new kind of job here, can we use bundler instead? Comment at: clang/lib/Code