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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
