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

[Joseph Huber via Phabricator via llvm-commits]

jhuber6 added a comment.

In D133539#3955099 <https://reviews.llvm.org/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