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 to an object by the clang at compile time. I not sure how you can guarantee that symbol names will match.

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 clang, there are no changes in this area. What is moving to a wrapper tool is the generation of the offload registration code. Let me just attach the slides that I presented on the inter company meeting were the proposal was discussed. It'll probably answer most of your questions.

openmp_linker_script.pdf391 KBDownload

I'm on board with getting rid of the linker script. Gold's limited support for that seems conclusive.

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 instead of hidden. I guess it will cause problems for offload programs that use shared libraries because DSO’s __start_name/__stop_name symbols will be preempted by the executable’s symbols and that is not what we want. Is there any way to change this behavior?

In D64943#1666849, @sdmitriev wrote:

In D64943#1666636, @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 __omp_offloading_entries in which case the linker will provide start/end symbols automatically?

Well, I never said that it is an ideal solution, but it is a known mechanism that works well in many cases and can also be reused for the offloading entry table.
I do not fully understand your suggestion for renaming entries section, how it will help with providing start/end symbols for the entries. Can you please provide more details?

In D64943#1666679, @ABataev wrote:

In D64943#1666678, @JonChesterfield wrote:

In D64943#1666673, @ABataev wrote:

In D64943#1666658, @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 with the linker script? I'm wondering if they can be resolved without the overhead of introducing a new tool.

They just do not support linker script. And, thus, cannot be used for offloading. Only ld supports it.

In what respect? I've used linker scripts with both gold and lld, and both instances of --help text claim to support them. In the case of lld, a very complicated script hit a few internal errors, but I believe they've all been fixed since.

Hmm, I tried it with gold some time ago and it just did not work for me. The linking failed with diagnostics that some of the commands in the script are unknown.

In D64943#1666673, @ABataev wrote:

In D64943#1666658, @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 with the linker script? I'm wondering if they can be resolved without the overhead of introducing a new tool.

They just do not support linker script. And, thus, cannot be used for offloading. Only ld supports it.

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

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 automatically? That removes the two object files and the link order dependency which is a hazard to bitcode libraries.

Thanks for the explicit asm. One of the hazards of compiling cuda for amdgcn is that volatile doesn't imply atomic, so this is a clear warning that I'll have a bug on merge.

Looks good here. Apologies for the rebase induced by the recent patches to omptarget-nvptx.

Thanks guys. It's nice to see the last function style macros disappear from omptarget-nvptx. This means all the CUDA_VERSION tests are now localised to target_impl.h,

Replace types with __kmpc_impl_lanemask_t wherever they are used within a function.

• keep ULL type in IsWarpMasterActiveThread

• uint32 -> lanemask_t

• rebase

Phabricator has decided to mix this patch in with the other patches that have landed on master since initially creating it. Presumably my mistake for rebasing. I'll abandon this and create a new diff. Sorry for the noise.

• remove fixme, add comment, rebase on master

Thanks. I wasn't sure whether to leave the FIXMEs in place or not, will drop them.

Abandoning in favour of split revisions (one per macro)

• comment end of #if block

• drop spurious return

Will split as recommended

• from review - fix bug when compiling with clang

I'm not sure about the signal to noise of the C style __kmpc_impl namespace. Perhaps C++ namespace __kmpc_impl {} and using namespace __kmpc_impl; in the source would be better?

Looks good. Delighted to see extensions to target_impl.

In D65836#1628764, @Hahnfeld wrote:

Jon, please subscribe to openmp-commits so that commit emails get through immediately. Thanks!

What about namong convention here? Shall we use capital letters for the var names or it is fine as is?

Long term it's probably worth moving to the LLVM conventions throughout. Short term, I'd rather keep roughly the same style as the surrounding code. I believe that's what this patch does.

Ping. I'd like to land this - Alexey?

I'm not sure about this diff. I think it's breaking <iostream> and <algorithm>. Raised bug https://bugs.llvm.org/show_bug.cgi?id=42972

I'm wary of refactoring to support multiple architectures while there is only one in tree. It's too difficult to see where the abstractions should be, and it's usually easier to introduce said abstractions than to move them later.

I like this a lot. Thanks

• drop omptarget-nvptx include
• address review comments
• rebase, use INLINE from D65876

In D65836#1618858, @ABataev wrote:

I would suggest at first to come to an agreement on the design of this reworked library at first.

• address review comments

• drop omptarget-nvptx include

Couple of comments from me inline.

I've implemented this out of tree (for Graphcore), based loosely on the PPC implementation. IR pass based on SCEV inserts intrinsics, SDag patches them up a little, MIR pass picks appropriate instructions or falls back to a decrement and branch loop.

It looks like the changes to support/commandline are useful independent of the file check use case. Would it be worth submitting the command line enhancement by itself before updating filecheck to take advantage?

Thank you. I've just seen the buildbot failure but you've beaten me to the patch. Embarrassing error, sorry.

In D34685#813125, @ruiu wrote:

LGTM

In D34685#796846, @ruiu wrote:

You can distinguish by filename, as target-specific tests start with target name.

In D34685#796778, @ruiu wrote:

I'm still waiting for you to update this patch to address Rafael's comment.

Thanks Paul. This is my first time looking at lld, the guidance is appreciated.