[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