[Openmp-commits] [clang] [llvm] [openmp] [OpenMP] Increment kernel args version, used by runtime for detecting dyn_ptr. (PR #85363)

[Jeff Sandoval via Openmp-commits]

jeffreysandoval wrote:

> > > The versioning is required to support use cases where code generated by an older compiler is linked with a newer runtime.
> > 
> > 
> > Is that supported?
> 
> I think compatibility across released versions is not supported in upstream LLVM. But downstream, there are use cases. For example, classic-flang that does not generate the implicit parameter. While we maintained this change downstream for some time, it would be very helpful for maintenance if this could be merged upstream. The change in this patch is fairly simple and non-intrusive.

We're hitting this with HPE's compiler/library, too, because adding the extra kernel argument without explicitly communicating it to libomptarget breaks the compiler-library __tgt ABI.  I think a version increment is expected for a change like this.  It's the kind of change that may not be a problem for long, assuming all LLVM-based compilers eventually follow the new policy of adding the extra argument.  But, until that happens the transition can be very difficult for users that mix a variety of compilers.  E.g., right now HPE's libcrayacc (which implements the __tgt interface) cannot "get it right" for all compilers of interest, since vanilla upstream expects the extra argument to be added but AMD ROCm does not.  Incrementing the version allows the library to determine whether the argument needs to be added.

This seems like the most expedient/straightforward fix.  Another option is to add an explicit `KernelArgsTy::Flags` field, though that probably requires a version increment, too (unless we're guaranteed that unused flags are always initialized to 0).  Or, a different approach would be to add a new OMP_TGT_MAPTYPE enum value (e.g., OMP_TGT_MAPTYPE_TARGET_PARAM_DYN_PTR) that would allow the compiler to explicitly represent this extra kernel argument in the normal argument list.  The library would add special-case handling to fill in the launch env pointer for this kernel argument.  These options would be more implementation effort, so I propose moving forward with this patch first.  But if there is interest in these other approaches, they could be investigated later.  These mechanisms could be used to avoid the need for the `isCtorOrDtor()` check before adding the extra kernel argument in libomptarget (i.e., the launch of a Ctor or Dtor kernel could explicitly specify that a kernel launch env is not needed).

https://github.com/llvm/llvm-project/pull/85363