Removed Triple format example from documentation and simplified conditional calling of bundling/unbundling functions.

Removed unused header.

1. Removed TargetID support, to be reviewed in a followup patch.
2. Added OffloadTargetInfo class to encapsulate handling of bundle entry ID components: OffloadKind, Triple, GPUArch.

Added support for optional TargetID during unbundling of archives.

LGTM. But, please wait from someone outside AMD to accept it.

In D93525#2495937, @t-tye wrote:

In D93525#2495374, @saiislam wrote:

In D93525#2493752, @t-tye wrote:

In D93525#2493024, @yaxunl wrote:

can you document this in ClangOffloadBundler.rst ? I think we need a clear description about how clang-offload-bundler knows which file in the .a file belongs to which target.

How does the .a relate to bundled code objects? Does the .a have a number of bundled code objects? If so wouldn't the identity of code objects be defined by the existing bundled code object ABI already documented? If the .a is a set of non-bundled code objects then defining how they are identified is not part of the clang-offload-bundler documentation as there are no bundled code objects involved. It would seem that the documentation belongs with the OpenMP runtime/compiler that is choosing to use .a files in this manner.

Bundles (created using clang-offload-bundler) are passed to llvm-ar to create an archive of bundled objects (*.a file). An archive can have bundles for multiple device types. So, yes, the identity of code objects is defined by the existing bundled code object ABI.
This patch reads such an archive and produces a device-specific archive for each of the target devices given as input. Each device-specific archive contains all the code objects corresponding to that particular device and are written as per llvm archive format.

Here is a snippet of relevant lit run lines:

// RUN: %clang -O0 -target %itanium_abi_triple %s -c -o %t.o

// RUN: echo 'Content of device file 1' > %t.tgt1
// RUN: clang-offload-bundler -type=o -targets=host-%itanium_abi_triple,openmp-amdgcn-amd-amdhsa-gfx900 -inputs=%t.o,%t.tgt1 -outputs=%t.abundle1.o
 
// RUN: echo 'Content of device file 2' > %t.tgt2
// RUN: clang-offload-bundler -type=o -targets=host-%itanium_abi_triple,openmp-amdgcn-amd-amdhsa-gfx900 -inputs=%t.o,%t.tgt2 -outputs=%t.abundle2.o
 
// RUN: llvm-ar cr %t.lib.a %t.abundle1.o %t.abundle2.o

This patch ==>
// RUN: clang-offload-bundler -unbundle -type=a -targets=openmp-amdgcn-amd-amdhsa-gfx900 -inputs=%t.lib.a -outputs=%t.devicelib.a

%t.devicelib.a will contain all devices objects corresponding to gfx900

Though my interest originates from OpenMP side, Device-specific Archive Libraries created like this can be used by other offloading languages like HIP, CUDA, and OpenCL. Pelase refer D81109 for the an earlier patch in the series of patches which will enable this.

The naming of code objects in a bundled code object includes the processor name and the settings for target features (see https://clang.llvm.org/docs/ClangOffloadBundler.html#target-id and https://llvm.org/docs/AMDGPUUsage.html#target-id). The compatibility of code objects considers both target processor matching and target feature compatibility. Target features can have three settings: on, off and any. The compatibility is that each feature that is on/off must exactly match, but any will match either on or off.

So when unbundling an archive how is the desired code object being requested? How is it handling the target features? For example, if code objects that will be compatible with a feature being on is required, then matching code objects in the archive would be those that have that feature either on or any.

In D93525#2493752, @t-tye wrote:

In D93525#2493024, @yaxunl wrote:

can you document this in ClangOffloadBundler.rst ? I think we need a clear description about how clang-offload-bundler knows which file in the .a file belongs to which target.

How does the .a relate to bundled code objects? Does the .a have a number of bundled code objects? If so wouldn't the identity of code objects be defined by the existing bundled code object ABI already documented? If the .a is a set of non-bundled code objects then defining how they are identified is not part of the clang-offload-bundler documentation as there are no bundled code objects involved. It would seem that the documentation belongs with the OpenMP runtime/compiler that is choosing to use .a files in this manner.

Ping.

Thank you, @sdmitriev. I have updated the patch to handle multiple targets/outputs in one run of the tool (https://reviews.llvm.org/D93525?id=314633). Please have a look.

Modified to handle multiple targets/outputs in one run of the tool for archive unbundling. Other minor changes as requested in the review.

In D92535#2455987, @tra wrote:

The change look OK. But we should still wait for @jsjodin to confirm that the first-file-is-different for archive files is unintentional.

In D92535#2469246, @sdmitriev wrote:

Well, it looks like nobody is going to respond.

Refer to D80816 for earlier review of this patch.

As @jsjodin is not working on this anymore, I have taken it up in a new patch, D93525 (didn't like the commandeering option).
New clang-formatted patch has a couple of new test cases to elaborate its working further.

1. Simplifies overall patch after D90248.
2. Removes MaxParallelLevel and thus target specific PrePostActionTy.
3. Removes ExternallyInitialized qualifier from shared variables for AMDGCN.

Ping.

Ping.
Merged patch D87841 uses the Gridvalue as described here.

1. Removed unnecessary formatting of untouched code.
2. Encapsulated addFieldToRecordDecl and createGlobalStruct methods in a class and made them static (triggered change at all calling sites).
3. Marked most of the member methods of CGOpenMPRuntimeAMDGCN as private (forgot to do same change in nvptx)
4. Fixed the memory leak
5. Marked appropriate member variables as protected in CGOpenMPRuntimeGPU

AFAIK, all files which are required/should be available in libomptarget reside in llvm/Frontend/OpenMP/* , so may be an option can be provided for only-tarball users to get files only from this directory.
This kind of approach will also enforce people exporting stuff from llvm to libomptarget, to put everything in one place.

Ping.

Moved the code to load plugins to a helper function and before loop on plugins.

First of two patches after splitting D73657

Ping.

In D85214#2243160, @gribozavr2 wrote:

@saiislam This change seems to have broken a buildbot, please take a look at your earliest convenience.

Before: http://lab.llvm.org:8011/builders/clang-x86_64-debian-fast/builds/35354

After: http://lab.llvm.org:8011/builders/clang-x86_64-debian-fast/builds/35355

1. Reformarting
2. Comments
3. Reduced scope of specialized PrePostActionTy

1. Moved amdgcn specific functions to CGOpenMPAMDGCN.cpp
2. Removed tautology condition
3. Corrected case of local variables
4. Restored original formatting
5. Changed back declaration of emit kernel methods as private
6. Added support of amdgcn specific PrePostActionTy implementation and its corresponding test cases
7. Changed static line numbers in new test cases with regex
8. Other small code corrections

In D86232#2226668, @JonChesterfield wrote:

This can't be used in the libomptarget plugins or in the deviceRTL, as both are required to compile without the llvm sources available.

Current preferences appears to be to copy and paste the numbers between the two files. At the least, we should have a test outside of llvm & openmp trees that verifies the numbers are the same in each.

@jdoerfert can you remember who was strongly against #include between openmp and llvm? I'm failing to find the discussion, and the mail "Sharing enums between llvm and openmp" didn't reach a conclusion

Please let me know if there are any changes/suggestions.

Time for check-clang has increased from ~51 sec to ~107 sec after this patch.
Putting it here before committing/closing the review.

Ping.

Ping.

Ping for this (part-1), D85150 (part-2), and D85214 (part-3).

Next two (independent) parts are: D85150 and D85214. More to come.

Abandoning in favor of D85214

This patch updates 101 out 320 test files. Remaining will be posted as a separate patch to ease review.

Abandoning in favor of D84844 and D85150 i.e. adding default version testing along with version 4.5 testing.

Removed redundant testing for version 5.0 with its version string.

Changes as requested.

Improved comment placement. Corrected check lines. Rebased.

Moved comments to header files. Used regexps for test cases.

Generalized regex in one of the test to pass harbormaster build.

Added InitTempAlloca address space cast.

LGTM. Please wait for @arsenm 's review.

Added final to specialized classes.

Thank you. LGTM.

It is causing various recent OpenMP harbormaster builds to fail like the one for D84148 and D84221.

Can you add test with version string 50 also?
So, tests should be there for version 45, version 50, and default version. It will ensure that this test doesn't silently stops testing for 50 when we upgrade to the next version.

In D64571#2162388, @ABataev wrote:

In D64571#2160362, @protze.joachim wrote:

The test fails for me with segfault in line 22.

$ gdb target_depend_nowait.cpp.tmp-x86_64-pc-linux-gnu
(gdb) run
...
Program received signal SIGSEGV, Segmentation fault.
(gdb) bt
#0  0x0000000000400fcf in .omp_outlined._debug__ (.global_tid.=0x2aaab96b9be0, .bound_tid.=0x2aaab96b9bd8) at target_depend_nowait.cpp:22
#1  0x0000000000401e8d in .omp_outlined..23 (.global_tid.=0x2aaab96b9be0, .bound_tid.=0x2aaab96b9bd8) at target_depend_nowait.cpp:18
#2  0x00002aaaab574213 in __kmp_invoke_microtask () from libomp.so
#3  0x00002aaaab51338e in __kmp_invoke_task_func () from libomp.so
#4  0x00002aaaab5126bf in __kmp_launch_thread () from libomp.so
#5  0x00002aaaab55d3d0 in __kmp_launch_worker(void*) () from libomp.so
#6  0x00002aaaabbd6e65 in start_thread () from libpthread.so.0
#7  0x00002aaaabee988d in clone () from libc.so.6

It is not the problem of this patch, looks like support for mappers causes failures

Corrected.

Removed getNVPTXWarpSize() changes for a separate patch and rebased.

In D83863#2153031, @ABataev wrote:

In D83863#2153021, @saiislam wrote:

In D83863#2153008, @ABataev wrote:

Not sure that this is really required. If we want to check for the default version, it would be good to check that it still works for 4.5. It means, need to add additional RUN lines. But I don't think it is profitable to test the same functionality several times without actual effect

These 67 files were testing only for version 4.5. As the default version is now 5.0, isn't it expected that the tests run against it?

It just means that there are no changes in functionality between 4.5 and 5.0. Sure, you can switch to the default version instead but I don't see real profit here. Anyway, it is up to you.

In D83863#2153008, @ABataev wrote:

Not sure that this is really required. If we want to check for the default version, it would be good to check that it still works for 4.5. It means, need to add additional RUN lines. But I don't think it is profitable to test the same functionality several times without actual effect

[Work in progress]
Among all the test files in clang/test/OpenMP which were running only for version 4.5, I am yet to fix following files (rest all are given in this patch):

Instead of adding additional test cases without version string, removed all occurrences of -fopenmp-version=50.

Thank you.
I will hold committing till llvm-11 branching so it doesn't break something in any downstream dependent project at the last moment.