[Openmp-commits] [PATCH] D138495: [openmp][mlir] Lower parallel if to new fork_call_if function.

Wed Dec 14 06:16:36 PST 2022

kiranchandramohan added inline comments.

================
Comment at: openmp/runtime/src/kmp.h:3904
                                  kmpc_micro microtask, ...);
+KMP_EXPORT void __kmpc_fork_call_if(ident_t *loc, kmp_int32 nargs,
+                                    kmpc_micro microtask, kmp_int32 cond,
----------------
mstorsjo wrote:
> kiranchandramohan wrote:
> > mstorsjo wrote:
> > > mstorsjo wrote:
> > > > mstorsjo wrote:
> > > > > DavidTruby wrote:
> > > > > > mstorsjo wrote:
> > > > > > > This new function needs to be added to the `dllexports` file to make it available and usable on Windows.
> > > > > > Ok, I can add that; do you have any clue how the numbers for functions are chosen in that file? I don't really understand how dll exporting works on Windows
> > > > > In general cases, it would be enough to just add the name (or mark the symbol with a `dllexport` attribute in the source code), but here in OpenMP, the exports also are given ordinal numbers (which need to be unique). I think the logic so far is to just pick the next free number.
> > > > > 
> > > > > D131830 is adding `292` and `293` (if it ends up reapplied in its current form), so I guess we could go with `294` to avoid conflicts - unless anyone of the actual OpenMP maintainers have a better suggestion?
> > > > CC @tianshilei1992 who has sorted out such issues before too.
> > > > 
> > > > I'd like to have this issue solved soonish in one way or another - this has had my nightly builds broken for many days already.
> > > What do the maintainers, who I hope are reading this, prefer here - that I push a patch that just adds this function to `dllexports` with a free ordinal number picked for the function - or revert the patch that was applied last week so that we get it fixed with proper process?
> > @DavidTruby was/is not working today. If you can add a fix that would be great.
> I pushed a fix for this in 15151315f76b0840423bd3bd62c5f9fc647d31c6 now.
Thanks @mstorsjo.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138495