[Openmp-commits] [PATCH] D137168: [OpenMP][mingw] Fix build for aarch64 target

Wed Nov 2 00:58:57 PDT 2022

alvinhochun added a reviewer: AndreyChurbanov.
alvinhochun added inline comments.

================
Comment at: openmp/runtime/src/dllexports:1265
     __kmpc_atomic_val_8_cas_cpt            2158
+    %endif

----------------
mstorsjo wrote:
> mstorsjo wrote:
> > natgla wrote:
> > > branh wrote:
> > > > mstorsjo wrote:
> > > > > @natgla - What about this change - was this something which was missed/forgotten and/or just not yet upstreamed from your work on openmp on MSVC/arm64?
> > > > We haven't yet implemented OpenMP 5.1 on MSVC. We're finishing up OpenMP 3.1 support now.
> > > > 
> > > > For OpenMP 3.1, we did change this file to allow the various flavors of __kmpc_atomic_<type>_<opt>_cpt in the OpenMP 3.1 section on arm64. LLVM inlines the atomic lock acquisition/etc. instead, and so doesn't need these calls, but we haven't implemented that in MSVC yet.
> > > > 
> > > > It looks like your change is *removing* declarations for certain _cpt and _cas functions on arm64. If you expected us to need these functions on MSVC/arm64, wouldn't the current state be what we need? 
> > > this part wasn't broken for us - because libomp140 is still based on LLVM 11.
> > Ah, I see, these functions were added after that patch (4fb0aaf03381473ec8af727edb4b5d59b64b0d60 / D101173) was upstreamed.
> Currently, linking of the OpenMP DLL fails, since this list declares that these functions are to be exported - but since there's no implementation of them for Windows/ARM64, linking fails. This part of the patch excludes them from being exported.
> 
> If it's intended to implement them later, I guess this should be a separate ifdef block, with a comment saying that this just is a temporary measure and that they are expected to be implemented later.
I excluded these functions based on D110109 (which implements them) stating they are x86/x86_64-only.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137168