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

[Natalia Glagoleva via Phabricator via Openmp-commits]

natgla added inline comments.


================
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);
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137828