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

[Martin Storsjö via Phabricator via Openmp-commits]

mstorsjo added inline comments.


================
Comment at: openmp/runtime/src/dllexports:1265
     __kmpc_atomic_val_8_cas_cpt            2158
+    %endif
 
----------------
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.


================
Comment at: openmp/runtime/src/dllexports:1265
     __kmpc_atomic_val_8_cas_cpt            2158
+    %endif
 
----------------
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.


================
Comment at: openmp/runtime/src/kmp_os.h:621
 
+#elif KMP_ARCH_AARCH64 && defined(__GNUC__)
+
----------------
natgla wrote:
> mstorsjo wrote:
> > natgla wrote:
> > > For MSVC we compiled libomp only with MSVC, so the patch is a bit different. @branh will start upstreaming the changes later this month, they will be complimentary to this change. 
> > > Am I right that this patch will fix the issues only if compiled with gcc?
> > > looks good to me.
> > This patch fixes issues if compiled with clang in mingw mode - gcc doesn't support windows on aarch64 (yet).
> Is this: "defined(__ GNUC __)" for mingw then? 
Yes, that's the correct compiler define to look for; clang identifies itself both as `__GNUC__` and `__clang__` on most platforms (except for MSVC targets) - it does that for mingw targets too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137168