[Openmp-commits] [PATCH] D137828: RFC: [openmp] Don't assume a specific layout for alloca() in Windows arm64 __kmp_invoke_microtask

[Martin Storsjö via Phabricator via Openmp-commits]

mstorsjo added a comment.

In D137828#3928117 <https://reviews.llvm.org/D137828#3928117>, @natgla wrote:

> @mstorsjo, sorry, didn't ask you earlier: what is your environment? Our combination: everything is on Windows; we are using MSVC compiler to compile OpenMP code (for x64, x86, or arm64), and using libomp140 for runtime. LLVM libomp runtime is compiled by MSVC (for x64, x86, arm64) with additional definition 'OPENMP_MSVC_NAME_SCHEME' for cmake. We also have LLVM_ENABLE_PDB=ON for cmake for release, AFAIK it's different from usual.

I build libomp with Clang, for i686, x86_64 and aarch64 (in mingw mode, but I test some aspects occasionally with building with clang in MSVC mode too).

I then run the openmp lit testsuite on Windows on arm64 (and on x86_64 too).



================
Comment at: openmp/runtime/src/z_Windows_NT-586_util.cpp:152
   switch (argc) {
+  default:
+    fprintf(stderr, "Too many args to microtask: %d!\n", argc);
----------------
natgla wrote:
> mstorsjo wrote:
> > pulidocr wrote:
> > > Is there a fallback path for ARM64 MSVC? If there isn't, then our libomp140.aarch64.dll might break. @natgla FYI
> > No, there's no fallback path to that. So yes, this would break your libomp builds for the cases if passing more than 15 arguments here.
> > 
> > But the main point is that the `alloca` + `memcpy` construct is extremely brittle here - I wouldn't even be sure that an update of MSVC could break it (since if you look at it from a compiler point of view, the `memcpy` is a dead write to a buffer which is not referenced anywhere).
> > 
> > As far as I know, the only properly robust solution, for an arbitrary number of arguments, is to write this function in assembly (like I do with gnu assembly in D137827). I guess it's possible to port the existing aarch assembly function to MS armasm64 syntax too (but I don't know if cmake knows how to interact with that tool).
> > 
> @mstorsjo,  
> For now please keep the current implementation for MSVC - or re-write it in assembler. Yes, it's brittle, it will be great to switch to an array of arguments instead of using varargs, but we don't have resources now. Currently it's working. It may break if there will be changes in __kmp_invoke_microtask or if MSVC compiler will get better with optimizations. We have tests to ensure we won't break it in Microsoft, and if we'll do, we'll have to re-write this function. And we plan to add our testing bot to LLVM.
Ok, I’ll abandon this patch then. D137827 replaces this function with an assembler implementation - but the implementation is gnu assembly, so it works with clang, but not with MSVC. (That patch keeps using this codepath for MSVC.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137828