Leftovers, if any, can be covered in a follow-up patch.

Ping

clang-formatted.

In D127769#3583335, @JonChesterfield wrote:

The control flow here is quite complicated and it's not immediately obvious why things are being stashed in a map. I vaguely recall the rules for choosing compatible images by target ID being complicated and undocumented.

Did you reverse engineer this from the HIP implementation, or is there now documentation for how the +/-/any stuff is meant to resolve?

Added comments for parseTargeID() .

Forgot to invert the condition in an if block.

In D126836#3554498, @josemonsalve2 wrote:

I have added the HSA_ISA_INFO_NAME, made the function static, and removed
the unused elements in the enums.

Could you please add support for HSA_ISA_INFO_NAME using hsa_isa_get_info_alt?
This one gives triple and target ID information, like amdgcn-amd-amdhsa--gfx908:sramecc-:xnack-

Added the multi-entry logic in libomptarget. Yet to move the image compatibility testing to plugin.

Thanks for the detailed review. I will update rest of the patch soon.

Addressed some simple review changes. Will update remaining in the next iteration.

Changed the embedding scheme to add ImageInfo field in __tgt_device_image.

Thanks!
LGTM.

Ideally we could just put this on the linker itself, but nvlink doesn't seem to support .a files.

In D124525#3491170, @jhuber6 wrote:

I'm suggesting instead we define a new __tgt_device_image and a new __tgt_register_lib function to support this. This new __tgt_device_image will simply contain a pointer to an optional information struct.

struct __tgt_device_image_v2 {
  void* 	ImageStart;
  void* 	ImageEnd;
  __tgt_offload_entry* 	EntriesBegin;
  __tgt_offload_entry* 	EntriesEnd;
  __tgt_image_into*  ImageInfo;
};

This new struct breaks the ABI with the old __tgt_device_image because these are put into an array and we change the size, but we should be able to provide backwards compatibility by copying from the old format to the new format and creating a new array. We can detect the new vs. old ABI by expecting that existing applications will call the __tgt_register_image function. We will create a new __tgt_register_image_v2 function for example that all new programs will call. In libomptarget we then change __tgt_register_image to do the necessary translation.

struct __tgt_bin_desc {                                                                                                                                                                  
  int32_t NumDeviceImages;           // Number of device types supported                                                                                                                   
  __tgt_device_image *DeviceImages;  // Array of device images (1 per dev. type)                                                        
  __tgt_offload_entry *HostEntriesBegin; // Begin of table with all host entries                                                           
  __tgt_offload_entry *HostEntriesEnd;   // End of table (non inclusive)                                                                                                           
};

EXTERN void __tgt_register_lib(__tgt_bin_desc *desc) {
  __tgt_device_image_v2 *new_image = alloc_new_version(desc);
  desc->DeviceImages = new_Image;
  __tgt_register_lib_v2(desc);
}

EXTERN void __tgt_unregister_lib(__tgt_bin_desc *desc) {
  __tgt_unregister_lib_v2(desc);
  dealloc(desc->DeviceImages);
}

Now the rest of libomptarget solely uses the new format, and we check if information is available by seeing that the ImageInfo field is non-null.

Looks good to me.

In D124525#3478469, @yaxunl wrote:

need a test for the generated registration code

In D123387#3439069, @yaxunl wrote:

Does this mean without this fix clang-offload-bundler emits an error if -outputs option not given? How could the lit tests passed?

Hey @jhuber6 , as discussed in multi-company meeting, I think that we will need at least an arch field somewhere in this. We would like to create multi-arch binaries so that runtime can load the compatible one on its own.
You may even consider using TargetID Format to store the list of archs.

LGTM

Will it be possible to use Bundle Entry ID format [1] for naming sections and being sort of the the primary key to refer an offload object?
The arch string following the triple will be just offload-arch (or march, or mcpu) for others, but for amdgpu it may contain some more information.

Restored device ancestor codegen unit test with requires reverese_offload.

Can you please add this tool's doc in clang/docs?

In rG876b5ea96bf5890074aec61bc2c6a37b2cdc0617#1054398, @MyDeveloperDay wrote:

ParseOpenMP.cpp was part of the "previously 100% clang-formatted files" ./clang/docs/tools/clang-formatted-files.txt

This commit contains code changes that are not clang-formatted

--- a/clang/lib/Parse/ParseOpenMP.cpp
+++ b/clang/lib/Parse/ParseOpenMP.cpp
@@ -2210,12 +2210,12 @@ Parser::DeclGroupPtrTy Parser::ParseOpenMPDeclarativeDirectiveWithExtDecl(
     VariantMatchInfo VMI;
     TI.getAsVariantMatchInfo(ASTCtx, VMI);

-    std::function<void(StringRef)> DiagUnknownTrait = [this, Loc](
-                                                          StringRef ISATrait) {
-      // TODO Track the selector locations in a way that is accessible here to
-      // improve the diagnostic location.
-      Diag(Loc, diag::warn_unknown_declare_variant_isa_trait) << ISATrait;
-    };
+    std::function<void(StringRef)> DiagUnknownTrait =
+        [this, Loc](StringRef ISATrait) {
+          // TODO Track the selector locations in a way that is accessible here
+          // to improve the diagnostic location.
+          Diag(Loc, diag::warn_unknown_declare_variant_isa_trait) << ISATrait;
+        };
     TargetOMPContext OMPCtx(
         ASTCtx, std::move(DiagUnknownTrait),
         /* CurrentFunctionDecl */ nullptr,
@@ -2551,12 +2551,12 @@ Parser::ParseOpenMPDeclarativeOrExecutableDirective(ParsedStmtContext StmtCtx) {
     TPA.Revert();
     // End of the first iteration. Parser is reset to the start of metadirective

-    std::function<void(StringRef)> DiagUnknownTrait = [this, Loc](
-                                                          StringRef ISATrait) {
-      // TODO Track the selector locations in a way that is accessible here to
-      // improve the diagnostic location.
-      Diag(Loc, diag::warn_unknown_declare_variant_isa_trait) << ISATrait;
-    };
+    std::function<void(StringRef)> DiagUnknownTrait =
+        [this, Loc](StringRef ISATrait) {
+          // TODO Track the selector locations in a way that is accessible here
+          // to improve the diagnostic location.
+          Diag(Loc, diag::warn_unknown_declare_variant_isa_trait) << ISATrait;
+        };
     TargetOMPContext OMPCtx(ASTContext, std::move(DiagUnknownTrait),
                             /* CurrentFunctionDecl */ nullptr,
                             ArrayRef<llvm::omp::TraitProperty>());

It seems that this patch along with D117156 and D117246 is giving patch application failed error [https://buildkite.com/llvm-project/diff-checks/builds/82688].
arc patch is also giving the same error.

Ping :-)

1. Used a common diagnostic warning warn_unknown_declare_variant_isa_trait for ParseOpenMP and SemaOpenMP for decalre variant and metadirectives.
2. Split lit codegen tests into two files, one requiring amdgpu-registered target and another for host only.
3. Added warning message lit test at an appropriate place.

Fixed the lit test failing in pre-check build bot.

Added diagnostic remarks for when ISA trait is not selected.

Added target specific tests for ISA traits, for CPU as well as GPU.

In D116549#3218281, @jdoerfert wrote:

Can you make the test check for the diagnose message? Also, do we have a test to verify an isa trait is properly handled?

In D111545#3218961, @MyDeveloperDay wrote:

Nit: That is more correct, but actually, you normally make the comment match the param name (I think there might even be a clang-tidy check for that?)

i.e.

/*isBitCodeSDL=*/

In D116540#3217689, @jdoerfert wrote:

In D116540#3217684, @saiislam wrote:

1. I don't know why "pgi" is present here as a vendor. Should it be renamed as nvidia instead? @Meinersbur @tra

It was in the spec list and I would not remove it now.

Specs 5.0 and 5.1 linked above doesn't contain PGI as a vendor. But, we can leave it as is.

1. Added nec as compiler vendor.
2. Fixed clang test cases.

1. I don't know why "pgi" is present here as a vendor. Should it be renamed as nvidia instead? @Meinersbur @tra
2. Should "cray" be renamed as "hpe" here? @sandoval
3. Should this vendor list be reordered as per Spec 5.1 [https://www.openmp.org/wp-content/uploads/OpenMP-API-Additional-Definitions-2-0.pdf] ?

Updated comments as per suggestion. Apologies for the delay.

clang-format(ed).

Thanks, Michael and Artem!

Used comment style for named parameters.

Fixed typo

1. Changed the option from path to nvlink-command.
2. Command line arguments are now parsed using proper API.

In rG35ebe4cc24f87397762e35831953c4bfe5f52def#1031080, @Meinersbur wrote:

This has broken the openmp-offload-cuda buildbots. For instance: http://meinersbur.de:8011/#/builders/1/builds/1516

******************** TEST 'libomptarget :: nvptx64-nvidia-cuda :: api/omp_get_num_devices.c' FAILED ********************
Script:
--
: 'RUN: at line 1';   /home/buildbot-worker/minipc-1050ti-linux/openmp-offload-cuda-runtime/llvm.build/./bin/clang -fopenmp  -fno-experimental-isel --cuda-path=/usr/local/cuda  -I /home/buildbot-worker/minipc-1050ti-linux/openmp-offload-cuda-runtime/llvm.src/openmp/libomptarget/test -I /home/buildbot-worker/minipc-1050ti-linux/openmp-offload-cuda-runtime/llvm.build/runtimes/runtimes-bins/openmp/runtime/src -L /home/buildbot-worker/minipc-1050ti-linux/openmp-offload-cuda-runtime/llvm.build/runtimes/runtimes-bins/openmp/libomptarget -L /home/buildbot-worker/minipc-1050ti-linux/openmp-offload-cuda-runtime/llvm.build/runtimes/runtimes-bins/openmp/runtime/src  -Wl,-rpath,/home/buildbot-worker/minipc-1050ti-linux/openmp-offload-cuda-runtime/llvm.build/runtimes/runtimes-bins/openmp/libomptarget -Wl,-rpath,/home/buildbot-worker/minipc-1050ti-linux/openmp-offload-cuda-runtime/llvm.build/runtimes/runtimes-bins/openmp/runtime/src -Wl,-rpath,/usr/local/cuda/lib64 --libomptarget-nvptx-bc-path=/home/buildbot-worker/minipc-1050ti-linux/openmp-offload-cuda-runtime/llvm.build/runtimes/runtimes-bins/openmp/libomptarget -fopenmp-targets=nvptx64-nvidia-cuda /home/buildbot-worker/minipc-1050ti-linux/openmp-offload-cuda-runtime/llvm.src/openmp/libomptarget/test/api/omp_get_num_devices.c -o /home/buildbot-worker/minipc-1050ti-linux/openmp-offload-cuda-runtime/llvm.build/runtimes/runtimes-bins/openmp/libomptarget/test/nvptx64-nvidia-cuda/api/Output/omp_get_num_devices.c.tmp && /home/buildbot-worker/minipc-1050ti-linux/openmp-offload-cuda-runtime/llvm.build/runtimes/runtimes-bins/openmp/libomptarget/test/nvptx64-nvidia-cuda/api/Output/omp_get_num_devices.c.tmp | /home/buildbot-worker/minipc-1050ti-linux/openmp-offload-cuda-runtime/llvm.build/./bin/FileCheck /home/buildbot-worker/minipc-1050ti-linux/openmp-offload-cuda-runtime/llvm.src/openmp/libomptarget/test/api/omp_get_num_devices.c
--
Exit Code: 1
Command Output (stdout):
--
$ ":" "RUN: at line 1"
note: command had no output on stdout or stderr
$ "/home/buildbot-worker/minipc-1050ti-linux/openmp-offload-cuda-runtime/llvm.build/./bin/clang" "-fopenmp" "-fno-experimental-isel" "--cuda-path=/usr/local/cuda" "-I" "/home/buildbot-worker/minipc-1050ti-linux/openmp-offload-cuda-runtime/llvm.src/openmp/libomptarget/test" "-I" "/home/buildbot-worker/minipc-1050ti-linux/openmp-offload-cuda-runtime/llvm.build/runtimes/runtimes-bins/openmp/runtime/src" "-L" "/home/buildbot-worker/minipc-1050ti-linux/openmp-offload-cuda-runtime/llvm.build/runtimes/runtimes-bins/openmp/libomptarget" "-L" "/home/buildbot-worker/minipc-1050ti-linux/openmp-offload-cuda-runtime/llvm.build/runtimes/runtimes-bins/openmp/runtime/src" "-Wl,-rpath,/home/buildbot-worker/minipc-1050ti-linux/openmp-offload-cuda-runtime/llvm.build/runtimes/runtimes-bins/openmp/libomptarget" "-Wl,-rpath,/home/buildbot-worker/minipc-1050ti-linux/openmp-offload-cuda-runtime/llvm.build/runtimes/runtimes-bins/openmp/runtime/src" "-Wl,-rpath,/usr/local/cuda/lib64" "--libomptarget-nvptx-bc-path=/home/buildbot-worker/minipc-1050ti-linux/openmp-offload-cuda-runtime/llvm.build/runtimes/runtimes-bins/openmp/libomptarget" "-fopenmp-targets=nvptx64-nvidia-cuda" "/home/buildbot-worker/minipc-1050ti-linux/openmp-offload-cuda-runtime/llvm.src/openmp/libomptarget/test/api/omp_get_num_devices.c" "-o" "/home/buildbot-worker/minipc-1050ti-linux/openmp-offload-cuda-runtime/llvm.build/runtimes/runtimes-bins/openmp/libomptarget/test/nvptx64-nvidia-cuda/api/Output/omp_get_num_devices.c.tmp"
# command stderr:
warning: linking module '/home/buildbot-worker/minipc-1050ti-linux/openmp-offload-cuda-runtime/llvm.build/runtimes/runtimes-bins/openmp/libomptarget/libomptarget-nvptx-sm_61.bc': Linking two modules of different target triples: '/home/buildbot-worker/minipc-1050ti-linux/openmp-offload-cuda-runtime/llvm.build/runtimes/runtimes-bins/openmp/libomptarget/libomptarget-nvptx-sm_61.bc' is 'nvptx64' whereas '/home/buildbot-worker/minipc-1050ti-linux/openmp-offload-cuda-runtime/llvm.src/openmp/libomptarget/test/api/omp_get_num_devices.c' is 'nvptx64-nvidia-cuda'
 [-Wlinker-warnings]
1 warning generated.
/home/buildbot-worker/minipc-1050ti-linux/openmp-offload-cuda-runtime/llvm.build/bin/clang-nvlink-wrapper: error: unable to find 'nvlink' in path
clang-14: error: nvlink command failed with exit code 1 (use -v to see invocation)
error: command failed with exit status: 1
--
********************

On the buildbot, nvlink is not in $PATH. Please use the nvlink executable in the path passed by --cuda-path

Thanks for reporting. Taking a look.

In D105191#3047954, @thakis wrote:

Fix didn't help, still broken at least on Windows: http://45.33.8.238/win/46515/step_7.txt