[Openmp-commits] [PATCH] D142593: [OpenMP] Support for wasm32 architecture

[arsnyder16 via Phabricator via Openmp-commits]

arsnyder16 added inline comments.


================
Comment at: openmp/runtime/src/kmp_os.h:1289-1290
+#elif KMP_ARCH_WASM
+#define KMP_DLSYM(name) nullptr
+#define KMP_DLSYM_NEXT(name) nullptr
 #else
----------------
penzn wrote:
> Emscripten has `dlopen` and `dlsym` emulation under a flag, though other ways to use Clang won't provide that.
Emulation is only supported if you are also compiling with dynamic linking, which isn't always the case. I am not sure how critical these code paths are to keep. 

https://github.com/emscripten-core/emscripten/blob/33a9cf1bceaaf52229285b6e77bf7f5747b18dba/src/library_dylink.js#L9


================
Comment at: openmp/runtime/src/kmp_platform.h:42
 #define KMP_OS_LINUX 1
-#elif (defined __linux__)
+#elif (defined __linux__) || defined(__EMSCRIPTEN__)
 #undef KMP_OS_LINUX
----------------
penzn wrote:
> Maybe it should be `__wasm32__` instead of `__EMSCRIPTEN__`, so that it is possible to build with non-Emscripten Clang (WASI-SDK, or even bare).
Possibly, but the Emscripten tool chain is the one supplying LINUX/POSIX (via musl).   Using just the clang compiler or other tool chains does not guarantee it will mimic a POSIX compatible system.  

Another option that could be explored is getting this working without  setting `KMP_OS_LINUX `  at all.  My guess would the main sticking point the code expecting to use pthreads if KMP_OS_LINUX  is defined.  I didn't actually try that i was looking for the path of least resistance, To make an official patch it might make sense to explore this further. 


================
Comment at: openmp/runtime/src/z_Linux_util.cpp:2450
+
+typedef void (*microtask_t0)(int *, int *);
+typedef void (*microtask_t1)(int *, int *, void*);
----------------
penzn wrote:
> Do you have to add those definitions because of how Wasm handles varargs?
Yea so this was the main runtime problem i encountered. I am little blurry on the exact details at it was about a year ago that i got this working. 

For starters function pointer can be problematic with emscripten https://emscripten.org/docs/porting/guidelines/function_pointer_issues.html

What i do recall is that when the address of the function was looked up in the function table it was always the same signature, so my assumption would be similar to yours its related to using a function pointer to function using varargs.   

We do have vargs in other areas of the the code base using libomp and i have yet to see any problem with those function. So it must be a combination of the function pointer and vargs.


Another interesting thing is that even recently i had to add even more microtasks signatures with more arguments (20-25). I wonder if the binaryen optimizer is adjusting call signatures in ways that causes these extra overloads. When I hit that problem it worked fine when using a debug build which would not have run through the optimizer. 


As a side, i find it interesting that the limit is 15 arguments. Is that a documented restriction or are there other things at a play that should never allow it beyond 15?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142593