Removed the unnecessary call to getArgs() and added test cases.

Ping.

Ping.

Thanks everyone for your review and comments!
I am going to address all of them in a series of smaller patches starting with D140784.

Fixed usage of CodeObjectVersion and ImplicitArgs size in the old plugin.
A separate patch will be required to support cov5 for the next gen plugin.

LGTM!

Thank you!
It is fixing our OpenMP smoke test failure.

The patch looks fine to me.
Please wait for @tra 's final review.

LGTM. Thank!

LGTM. Thanks!

I think we can not remove this initialization.

Apologies for the delay in response.

LGTM. Thanks!

In D131763#3719323, @jdoerfert wrote:

In D131763#3719140, @saiislam wrote:

In D131763#3719132, @jdoerfert wrote:

This doesn't actually test much, only once case/compilation is covered. In the second function nothing specific to LLVM as impl is checked.

The second function, is the only place in llvm-project where vendor(llvm) is being tested for a non-error test.

Really?

ag 'vendor\(llvm\)' clang/test/OpenMP --files-with-matches                                                                                                                                                                

clang/test/OpenMP/begin_declare_variant_messages.c
clang/test/OpenMP/begin_declare_variant_using_messages.cpp
clang/test/OpenMP/declare_variant_ast_print.c
clang/test/OpenMP/declare_variant_ast_print.cpp
clang/test/OpenMP/declare_variant_implementation_vendor_codegen.cpp
clang/test/OpenMP/declare_variant_messages.cpp
clang/test/OpenMP/declare_variant_mixed_codegen.cpp
clang/test/OpenMP/metadirective_ast_print.c
clang/test/OpenMP/metadirective_implementation_codegen.c
clang/test/OpenMP/nvptx_declare_variant_implementation_vendor_codegen.cpp
clang/test/OpenMP/declare_variant_messages.c
clang/test/OpenMP/metadirective_empty.cpp
clang/test/OpenMP/metadirective_implementation_codegen.cpp

That said, the above function still doesn't test anything wrt. llvm as impl anyway. We could just as well match amd or nvidia and the check lines still match just fine.

In D131763#3719132, @jdoerfert wrote:

This doesn't actually test much, only once case/compilation is covered. In the second function nothing specific to LLVM as impl is checked.

LGTM. Thanks!

In the multi-company OpenMP meeting, it was decided to defer this update.
So, please don't land this patch yet.

Updated with StringMap and StringRef ADTs. Also refactored based on new formating style.

In D128090#3648210, @jhuber6 wrote:

Sorry never noticed this revision. The purpose of this patch seems to be supporting something like this

clang input.c -fopenmp -fopenmp-targets=nvptx64 -Xopenmp-target=nvptx64 -march=sm_70 -Xopenmp-target=nvptx64 -march=sm_80

Right now the above works if you replace -march= with --offload-arch=. Currently the offloading tools use a "bound" architecture to tie a specific architecture with a job, which is what allows us to offload to multiple architectures. If there is no bound architecture gives, we instead use the -march= option, and if that is not present we derive it. It would be possible to set the bound architecture via -march if we wanted to. But I'm not sure if it's necessary given that it would just be an alternate syntax for --offload-arch=.

Thanks, LGTM!

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 :-)