[PATCH] D133539: [OpenMP] Replace OpenMP register requires constructor with a global array

[Joseph Huber via Phabricator via cfe-commits]

jhuber6 marked 2 inline comments as done.
jhuber6 added inline comments.



Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:10515
+default:
+  RequiresFlags.push_back(OMP_REQ_NONE);
+}

jdoerfert wrote:
> Really? Not an error or unexpected state?
Probably a good point. Would this be an unreachable? Since I think Sema should 
catch anything we don't expect here.



Comment at: openmp/libomptarget/src/rtl.cpp:360
+  if (Desc->Version >= 0)
 return;
 

jdoerfert wrote:
> Why do we allow to be called with an old struct, wouldn't we have converted 
> it at this point?
Leftover, will remove.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133539/new/

https://reviews.llvm.org/D133539

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D133539: [OpenMP] Replace OpenMP register requires constructor with a global array

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

Generally happy with this. Some minor comments. @sandoval, this works for you?




Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:3020-3025
+  if (!RequiresFlags.empty()) {
+for (int64_t Flag : RequiresFlags)
+  createRegisterRequires(CGM, Flag);
+  } else {
+createRegisterRequires(CGM, OMP_REQ_NONE);
+  }





Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:10515
+default:
+  RequiresFlags.push_back(OMP_REQ_NONE);
+}

Really? Not an error or unexpected state?



Comment at: openmp/libomptarget/src/rtl.cpp:360
+  if (Desc->Version >= 0)
 return;
 

Why do we allow to be called with an old struct, wouldn't we have converted it 
at this point?



Comment at: openmp/libomptarget/src/rtl.cpp:364
+   *RequiresE = Desc->RegisterRequiresEnd;
+   RequiresB != RequiresE; RequiresB++) {
+int64_t Flags = *RequiresB;

We have llvm::make_range, or similar


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133539/new/

https://reviews.llvm.org/D133539

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D133539: [OpenMP] Replace OpenMP register requires constructor with a global array

[Joseph Huber via Phabricator via cfe-commits]

jhuber6 added a comment.

In D133539#3955099 , @sandoval wrote:

> Overall this approach looks good to me, though I'd suggest some (hopefully 
> minor) changes that would help avoid breaking the ABI.  It looks like 
> `__tgt_bin_desc` is only used by `__tgt_[un]register_lib` -- rather than 
> changing those entry points, can you please add new ones (e.g., 
> `__tgt_[un]register_lib_v2` or `__tgt[un]register_lib_with_requires`)?  You 
> can still remove the old entry points from `libomptarget`.  This should avoid 
> any ABI issues with the old entry points, since calls to the old entry points 
> will expect the old struct and calls to the new entry points will expect the 
> new struct.  This will also make it possible for vendors to continue 
> supporting the old entry points, if desired.

Adding a new registration function would probably be the best way to maintain 
backwards compatibility, otherwise we'd segfault after reading past the struct. 
I can update this patch to do that.

> Also, would you mind renaming the `__tgt_bin_desc` struct to 
> `__tgt_bin_desc_v2` as well, to indicate it has changed?  This isn't part of 
> the ABI, so certainly not required, but it might help to convey that the 
> struct has changed from older versions of the source.
>
> Also, it looks like this patch only contains the compiler and test changes, 
> correct?  Is there another patch that contains the corresponding 
> `libomptarget` changes?

I wrote it but was waiting on this to get some attention before uploading it.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133539/new/

https://reviews.llvm.org/D133539

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D133539: [OpenMP] Replace OpenMP register requires constructor with a global array

[Jeff Sandoval via Phabricator via cfe-commits]

sandoval added a comment.

Overall this approach looks good to me, though I'd suggest some (hopefully 
minor) changes that would help avoid breaking the ABI.  It looks like 
`__tgt_bin_desc` is only used by `__tgt_[un]register_lib` -- rather than 
changing those entry points, can you please add new ones (e.g., 
`__tgt_[un]register_lib_v2` or `__tgt[un]register_lib_with_requires`)?  You can 
still remove the old entry points from `libomptarget`.  This should avoid any 
ABI issues with the old entry points, since calls to the old entry points will 
expect the old struct and calls to the new entry points will expect the new 
struct.  This will also make it possible for vendors to continue supporting the 
old entry points, if desired.

Also, would you mind renaming the `__tgt_bin_desc` struct to 
`__tgt_bin_desc_v2` as well, to indicate it has changed?  This isn't part of 
the ABI, so certainly not required, but it might help to convey that the struct 
has changed from older versions of the source.

Also, it looks like this patch only contains the compiler and test changes, 
correct?  Is there another patch that contains the corresponding `libomptarget` 
changes?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133539/new/

https://reviews.llvm.org/D133539

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D133539: [OpenMP] Replace OpenMP register requires constructor with a global array

[Joseph Huber via Phabricator via cfe-commits]

jhuber6 updated this revision to Diff 458948.
jhuber6 added a comment.

Fix LLVM test.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133539/new/

https://reviews.llvm.org/D133539

Files:
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  clang/lib/CodeGen/CGOpenMPRuntime.h
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/test/Driver/linker-wrapper-image.c
  clang/test/OpenMP/distribute_codegen.cpp
  clang/test/OpenMP/distribute_firstprivate_codegen.cpp
  clang/test/OpenMP/distribute_lastprivate_codegen.cpp
  clang/test/OpenMP/distribute_parallel_for_codegen.cpp
  clang/test/OpenMP/distribute_parallel_for_firstprivate_codegen.cpp
  clang/test/OpenMP/distribute_parallel_for_if_codegen.cpp
  clang/test/OpenMP/distribute_parallel_for_lastprivate_codegen.cpp
  clang/test/OpenMP/distribute_parallel_for_num_threads_codegen.cpp
  clang/test/OpenMP/distribute_parallel_for_private_codegen.cpp
  clang/test/OpenMP/distribute_parallel_for_proc_bind_codegen.cpp
  clang/test/OpenMP/distribute_parallel_for_simd_codegen.cpp
  clang/test/OpenMP/distribute_parallel_for_simd_firstprivate_codegen.cpp
  clang/test/OpenMP/distribute_parallel_for_simd_if_codegen.cpp
  clang/test/OpenMP/distribute_parallel_for_simd_lastprivate_codegen.cpp
  clang/test/OpenMP/distribute_parallel_for_simd_num_threads_codegen.cpp
  clang/test/OpenMP/distribute_parallel_for_simd_private_codegen.cpp
  clang/test/OpenMP/distribute_parallel_for_simd_proc_bind_codegen.cpp
  clang/test/OpenMP/distribute_private_codegen.cpp
  clang/test/OpenMP/distribute_simd_codegen.cpp
  clang/test/OpenMP/distribute_simd_firstprivate_codegen.cpp
  clang/test/OpenMP/distribute_simd_lastprivate_codegen.cpp
  clang/test/OpenMP/distribute_simd_private_codegen.cpp
  clang/test/OpenMP/distribute_simd_reduction_codegen.cpp
  clang/test/OpenMP/nvptx_lambda_capturing.cpp
  clang/test/OpenMP/openmp_offload_registration.cpp
  clang/test/OpenMP/reduction_implicit_map.cpp
  clang/test/OpenMP/target_codegen.cpp
  clang/test/OpenMP/target_codegen_global_capture.cpp
  clang/test/OpenMP/target_codegen_registration.cpp
  clang/test/OpenMP/target_depend_codegen.cpp
  clang/test/OpenMP/target_map_codegen_03.cpp
  clang/test/OpenMP/target_offload_mandatory_codegen.cpp
  clang/test/OpenMP/target_parallel_codegen.cpp
  clang/test/OpenMP/target_parallel_codegen_registration.cpp
  clang/test/OpenMP/target_parallel_depend_codegen.cpp
  clang/test/OpenMP/target_parallel_for_codegen.cpp
  clang/test/OpenMP/target_parallel_for_codegen_registration.cpp
  clang/test/OpenMP/target_parallel_for_depend_codegen.cpp
  clang/test/OpenMP/target_parallel_for_simd_codegen.cpp
  clang/test/OpenMP/target_parallel_for_simd_codegen_registration.cpp
  clang/test/OpenMP/target_parallel_for_simd_depend_codegen.cpp
  clang/test/OpenMP/target_parallel_if_codegen.cpp
  clang/test/OpenMP/target_parallel_num_threads_codegen.cpp
  clang/test/OpenMP/target_simd_codegen.cpp
  clang/test/OpenMP/target_simd_codegen_registration.cpp
  clang/test/OpenMP/target_simd_depend_codegen.cpp
  clang/test/OpenMP/target_teams_codegen.cpp
  clang/test/OpenMP/target_teams_codegen_registration.cpp
  clang/test/OpenMP/target_teams_depend_codegen.cpp
  clang/test/OpenMP/target_teams_distribute_codegen.cpp
  clang/test/OpenMP/target_teams_distribute_codegen_registration.cpp
  clang/test/OpenMP/target_teams_distribute_collapse_codegen.cpp
  clang/test/OpenMP/target_teams_distribute_depend_codegen.cpp
  clang/test/OpenMP/target_teams_distribute_dist_schedule_codegen.cpp
  clang/test/OpenMP/target_teams_distribute_firstprivate_codegen.cpp
  clang/test/OpenMP/target_teams_distribute_lastprivate_codegen.cpp
  clang/test/OpenMP/target_teams_distribute_parallel_for_codegen.cpp
  clang/test/OpenMP/target_teams_distribute_parallel_for_collapse_codegen.cpp
  clang/test/OpenMP/target_teams_distribute_parallel_for_depend_codegen.cpp
  
clang/test/OpenMP/target_teams_distribute_parallel_for_dist_schedule_codegen.cpp
  
clang/test/OpenMP/target_teams_distribute_parallel_for_firstprivate_codegen.cpp
  clang/test/OpenMP/target_teams_distribute_parallel_for_if_codegen.cpp
  clang/test/OpenMP/target_teams_distribute_parallel_for_lastprivate_codegen.cpp
  clang/test/OpenMP/target_teams_distribute_parallel_for_order_codegen.cpp
  clang/test/OpenMP/target_teams_distribute_parallel_for_private_codegen.cpp
  clang/test/OpenMP/target_teams_distribute_parallel_for_proc_bind_codegen.cpp
  clang/test/OpenMP/target_teams_distribute_parallel_for_reduction_codegen.cpp
  clang/test/OpenMP/target_teams_distribute_parallel_for_schedule_codegen.cpp
  clang/test/OpenMP/target_teams_distribute_parallel_for_simd_codegen.cpp
  
clang/test/OpenMP/target_teams_distribute_parallel_for_simd_codegen_registration.cpp
  
clang/test/OpenMP/target_teams_distribute_parallel_for_simd_collapse_codegen.cpp
  clang/test/OpenMP/target_teams_distribute_parallel_for_simd_depend_codegen.cpp
  

[PATCH] D133539: [OpenMP] Replace OpenMP register requires constructor with a global array

[Joseph Huber via Phabricator via cfe-commits]

jhuber6 updated this revision to Diff 458937.
jhuber6 added a comment.
Herald added subscribers: mattd, asavonic.

Fix tests


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133539/new/

https://reviews.llvm.org/D133539

Files:
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  clang/lib/CodeGen/CGOpenMPRuntime.h
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/test/Driver/linker-wrapper-image.c
  clang/test/OpenMP/distribute_codegen.cpp
  clang/test/OpenMP/distribute_firstprivate_codegen.cpp
  clang/test/OpenMP/distribute_lastprivate_codegen.cpp
  clang/test/OpenMP/distribute_parallel_for_codegen.cpp
  clang/test/OpenMP/distribute_parallel_for_firstprivate_codegen.cpp
  clang/test/OpenMP/distribute_parallel_for_if_codegen.cpp
  clang/test/OpenMP/distribute_parallel_for_lastprivate_codegen.cpp
  clang/test/OpenMP/distribute_parallel_for_num_threads_codegen.cpp
  clang/test/OpenMP/distribute_parallel_for_private_codegen.cpp
  clang/test/OpenMP/distribute_parallel_for_proc_bind_codegen.cpp
  clang/test/OpenMP/distribute_parallel_for_simd_codegen.cpp
  clang/test/OpenMP/distribute_parallel_for_simd_firstprivate_codegen.cpp
  clang/test/OpenMP/distribute_parallel_for_simd_if_codegen.cpp
  clang/test/OpenMP/distribute_parallel_for_simd_lastprivate_codegen.cpp
  clang/test/OpenMP/distribute_parallel_for_simd_num_threads_codegen.cpp
  clang/test/OpenMP/distribute_parallel_for_simd_private_codegen.cpp
  clang/test/OpenMP/distribute_parallel_for_simd_proc_bind_codegen.cpp
  clang/test/OpenMP/distribute_private_codegen.cpp
  clang/test/OpenMP/distribute_simd_codegen.cpp
  clang/test/OpenMP/distribute_simd_firstprivate_codegen.cpp
  clang/test/OpenMP/distribute_simd_lastprivate_codegen.cpp
  clang/test/OpenMP/distribute_simd_private_codegen.cpp
  clang/test/OpenMP/distribute_simd_reduction_codegen.cpp
  clang/test/OpenMP/nvptx_lambda_capturing.cpp
  clang/test/OpenMP/openmp_offload_registration.cpp
  clang/test/OpenMP/reduction_implicit_map.cpp
  clang/test/OpenMP/target_codegen.cpp
  clang/test/OpenMP/target_codegen_global_capture.cpp
  clang/test/OpenMP/target_codegen_registration.cpp
  clang/test/OpenMP/target_depend_codegen.cpp
  clang/test/OpenMP/target_map_codegen_03.cpp
  clang/test/OpenMP/target_offload_mandatory_codegen.cpp
  clang/test/OpenMP/target_parallel_codegen.cpp
  clang/test/OpenMP/target_parallel_codegen_registration.cpp
  clang/test/OpenMP/target_parallel_depend_codegen.cpp
  clang/test/OpenMP/target_parallel_for_codegen.cpp
  clang/test/OpenMP/target_parallel_for_codegen_registration.cpp
  clang/test/OpenMP/target_parallel_for_depend_codegen.cpp
  clang/test/OpenMP/target_parallel_for_simd_codegen.cpp
  clang/test/OpenMP/target_parallel_for_simd_codegen_registration.cpp
  clang/test/OpenMP/target_parallel_for_simd_depend_codegen.cpp
  clang/test/OpenMP/target_parallel_if_codegen.cpp
  clang/test/OpenMP/target_parallel_num_threads_codegen.cpp
  clang/test/OpenMP/target_simd_codegen.cpp
  clang/test/OpenMP/target_simd_codegen_registration.cpp
  clang/test/OpenMP/target_simd_depend_codegen.cpp
  clang/test/OpenMP/target_teams_codegen.cpp
  clang/test/OpenMP/target_teams_codegen_registration.cpp
  clang/test/OpenMP/target_teams_depend_codegen.cpp
  clang/test/OpenMP/target_teams_distribute_codegen.cpp
  clang/test/OpenMP/target_teams_distribute_codegen_registration.cpp
  clang/test/OpenMP/target_teams_distribute_collapse_codegen.cpp
  clang/test/OpenMP/target_teams_distribute_depend_codegen.cpp
  clang/test/OpenMP/target_teams_distribute_dist_schedule_codegen.cpp
  clang/test/OpenMP/target_teams_distribute_firstprivate_codegen.cpp
  clang/test/OpenMP/target_teams_distribute_lastprivate_codegen.cpp
  clang/test/OpenMP/target_teams_distribute_parallel_for_codegen.cpp
  clang/test/OpenMP/target_teams_distribute_parallel_for_collapse_codegen.cpp
  clang/test/OpenMP/target_teams_distribute_parallel_for_depend_codegen.cpp
  
clang/test/OpenMP/target_teams_distribute_parallel_for_dist_schedule_codegen.cpp
  
clang/test/OpenMP/target_teams_distribute_parallel_for_firstprivate_codegen.cpp
  clang/test/OpenMP/target_teams_distribute_parallel_for_if_codegen.cpp
  clang/test/OpenMP/target_teams_distribute_parallel_for_lastprivate_codegen.cpp
  clang/test/OpenMP/target_teams_distribute_parallel_for_order_codegen.cpp
  clang/test/OpenMP/target_teams_distribute_parallel_for_private_codegen.cpp
  clang/test/OpenMP/target_teams_distribute_parallel_for_proc_bind_codegen.cpp
  clang/test/OpenMP/target_teams_distribute_parallel_for_reduction_codegen.cpp
  clang/test/OpenMP/target_teams_distribute_parallel_for_schedule_codegen.cpp
  clang/test/OpenMP/target_teams_distribute_parallel_for_simd_codegen.cpp
  
clang/test/OpenMP/target_teams_distribute_parallel_for_simd_codegen_registration.cpp
  
clang/test/OpenMP/target_teams_distribute_parallel_for_simd_collapse_codegen.cpp
  clang/test/OpenMP/target_teams_distribute_parallel_for_simd_depend_codegen.cpp
  

[PATCH] D133539: [OpenMP] Replace OpenMP register requires constructor with a global array

[Joseph Huber via Phabricator via cfe-commits]

jhuber6 created this revision.
jhuber6 added reviewers: jdoerfert, JonChesterfield, tianshilei1992, ABataev, 
ronlieb, doru1004, RaviNarayanaswamy.
Herald added subscribers: guansong, yaxunl.
Herald added a project: All.
jhuber6 requested review of this revision.
Herald added subscribers: llvm-commits, cfe-commits, sstefan1.
Herald added projects: clang, LLVM.

Currently, every module containing OpenMP offloading code uses a global
constructor to register its flags from the `omp requires` clause. This
is not ideal as it creates several entry points into `libomptarget` that
requires the runtime to be initialized first. This makes it difficult
for us to transition to a scheme where the lifetime of `libomptarget` is
explicitly manages by the uses of the `__tgt_[un]register_lib` calls.

Instead, this patch changes the method to create global values similar
to how offloading entries are registered. The linker will create a
`__start/__stop` pointer for these values which we can then iterate
through in the runtime. This requires widening the `__tgt_bin_desc`
struct which is an ABI break as old programs  will attempt to read
invalid memory and the old interface will be removed. This will be
acceptable if we go through with the plans in D133277 
 and remove the
backwards compatibility in `libomptarget`.

If we do not wish to add a new field to `__tgt_bin_desc` we could
instead add it to the exiting `__tgt_offload_entry` and filter it out.
But this would require using the `Size` field or something similar to
hold the value and updating all the plugins.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D133539

Files:
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  clang/lib/CodeGen/CGOpenMPRuntime.h
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/test/Driver/linker-wrapper-image.c
  clang/test/OpenMP/distribute_codegen.cpp
  clang/test/OpenMP/distribute_firstprivate_codegen.cpp
  clang/test/OpenMP/distribute_lastprivate_codegen.cpp
  clang/test/OpenMP/distribute_parallel_for_codegen.cpp
  clang/test/OpenMP/distribute_parallel_for_firstprivate_codegen.cpp
  clang/test/OpenMP/distribute_parallel_for_if_codegen.cpp
  clang/test/OpenMP/distribute_parallel_for_lastprivate_codegen.cpp
  clang/test/OpenMP/distribute_parallel_for_num_threads_codegen.cpp
  clang/test/OpenMP/distribute_parallel_for_private_codegen.cpp
  clang/test/OpenMP/distribute_parallel_for_proc_bind_codegen.cpp
  clang/test/OpenMP/distribute_parallel_for_simd_codegen.cpp
  clang/test/OpenMP/distribute_parallel_for_simd_firstprivate_codegen.cpp
  clang/test/OpenMP/distribute_parallel_for_simd_if_codegen.cpp
  clang/test/OpenMP/distribute_parallel_for_simd_lastprivate_codegen.cpp
  clang/test/OpenMP/distribute_parallel_for_simd_num_threads_codegen.cpp
  clang/test/OpenMP/distribute_parallel_for_simd_private_codegen.cpp
  clang/test/OpenMP/distribute_parallel_for_simd_proc_bind_codegen.cpp
  clang/test/OpenMP/distribute_private_codegen.cpp
  clang/test/OpenMP/distribute_simd_codegen.cpp
  clang/test/OpenMP/distribute_simd_firstprivate_codegen.cpp
  clang/test/OpenMP/distribute_simd_lastprivate_codegen.cpp
  clang/test/OpenMP/distribute_simd_private_codegen.cpp
  clang/test/OpenMP/distribute_simd_reduction_codegen.cpp
  clang/test/OpenMP/openmp_offload_registration.cpp
  clang/test/OpenMP/reduction_implicit_map.cpp
  clang/test/OpenMP/target_codegen.cpp
  clang/test/OpenMP/target_codegen_global_capture.cpp
  clang/test/OpenMP/target_codegen_registration.cpp
  clang/test/OpenMP/target_depend_codegen.cpp
  clang/test/OpenMP/target_map_codegen_03.cpp
  clang/test/OpenMP/target_offload_mandatory_codegen.cpp
  clang/test/OpenMP/target_parallel_codegen.cpp
  clang/test/OpenMP/target_parallel_codegen_registration.cpp
  clang/test/OpenMP/target_parallel_depend_codegen.cpp
  clang/test/OpenMP/target_parallel_for_codegen.cpp
  clang/test/OpenMP/target_parallel_for_codegen_registration.cpp
  clang/test/OpenMP/target_parallel_for_depend_codegen.cpp
  clang/test/OpenMP/target_parallel_for_simd_codegen.cpp
  clang/test/OpenMP/target_parallel_for_simd_codegen_registration.cpp
  clang/test/OpenMP/target_parallel_for_simd_depend_codegen.cpp
  clang/test/OpenMP/target_parallel_if_codegen.cpp
  clang/test/OpenMP/target_parallel_num_threads_codegen.cpp
  clang/test/OpenMP/target_simd_codegen.cpp
  clang/test/OpenMP/target_simd_codegen_registration.cpp
  clang/test/OpenMP/target_simd_depend_codegen.cpp
  clang/test/OpenMP/target_teams_codegen.cpp
  clang/test/OpenMP/target_teams_codegen_registration.cpp
  clang/test/OpenMP/target_teams_depend_codegen.cpp
  clang/test/OpenMP/target_teams_distribute_codegen.cpp
  clang/test/OpenMP/target_teams_distribute_codegen_registration.cpp
  clang/test/OpenMP/target_teams_distribute_collapse_codegen.cpp
  clang/test/OpenMP/target_teams_distribute_depend_codegen.cpp
  clang/test/OpenMP/target_teams_distribute_dist_schedule_codegen.cpp