[Openmp-commits] [PATCH] D133053: [Libomptarget] Change device free routines to accept the allocation kind

Thu Sep 1 08:57:58 PDT 2022

tianshilei1992 added a comment.

In D133053#3764170 <https://reviews.llvm.org/D133053#3764170>, @JonChesterfield wrote:

> In D133053#3764152 <https://reviews.llvm.org/D133053#3764152>, @jhuber6 wrote:
>
>> In D133053#3764147 <https://reviews.llvm.org/D133053#3764147>, @tianshilei1992 wrote:
>>
>>> Is it stated somewhere? I had a lot of trouble about compatibility before when I tried to change the interface. I'm okay with breaking the backward compatibility, as long as we already tell users. :-)
>>
>> It's not too difficult to just make a new interface and have the old one call the new one with the default. Probably better safe than sorry so I may as well add it if it's a problem.
>
> It's simple to do but means we're stuck with all the cruft in the cuda plugin as we won't know which form of delete will be called.
>
> It'll be a really easy patch for downstream plugins to adopt (Intel has one iiuc?). The other use case is end users who have their plugin that they use with unrelated clang who I'm not sure exist. In either case, these toolchains will have already manually changed their libomptarget to get it to load their plugin already as we don't have any form of automatic discovery in place.
>
> I think we should change the calling convention and remove all the map lookup stuff from the cuda plugin unless we have a compelling reason not to, where miscellaneous concern about slightly breaking unknown users that are reliant on internal interfaces doesn't seem compelling.

You can still use `_v2` or whatever in `libomptarget`. For `libomptarget`, there is no old interface then. Just keep the old one and all the logic for compatibility. There is no confusion.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133053