Page MenuHomePhabricator

[OpenMP] Don't use MSVC workaround with MinGW
ClosedPublic

Authored by mati865 on Aug 4 2020, 7:12 AM.

Diff Detail

Event Timeline

mati865 created this revision.Aug 4 2020, 7:12 AM
mati865 requested review of this revision.Aug 4 2020, 7:12 AM
jdoerfert added a subscriber: AndreyChurbanov.

This looks fine to me but it doesn't impact me at all TBH.

@AndreyChurbanov WDYT?

AndreyChurbanov accepted this revision.Aug 4 2020, 7:51 AM

LGTM

Though I didn't test this as I don't have MinGW environment at hand.

This revision is now accepted and ready to land.Aug 4 2020, 7:51 AM

Little background:
Recent CMake versions changed behaviour on MinGW and treat .a as static libs only. Currently CMake creates omp.a as the import library and new CMake doesn't like that.

To fix it it's enough to do this change:

-set(LIBOMP_IMP_LIB_FILE ${LIBOMP_LIB_NAME}${CMAKE_STATIC_LIBRARY_SUFFIX})
+set(LIBOMP_IMP_LIB_FILE ${LIBOMP_LIB_NAME}${CMAKE_IMPORT_LIBRARY_SUFFIX})

But I've thought it'd be nice to avoid MSVC hack entirely when targeting MinGW.

At MSYS2 we are using older revision of this patch: https://github.com/msys2/MINGW-packages/commit/e071f276d31a8d214c2f30530eb9ac3ed44fffc7#diff-7f8e056c3dca04423132b5e4e1d36813

If you all agree could you commit the patch, please? I don't have permission to LLVM repo.

This revision was automatically updated to reflect the committed changes.
mstorsjo added a subscriber: hans.Aug 4 2020, 1:17 PM

@hans - I think this one might be cherrypick-worthy

hans added a comment.Aug 5 2020, 10:37 AM

@hans - I think this one might be cherrypick-worthy

Pushed to 11.x as d11e17309414b91c2e7dfc611f098041a62de201. Thanks!

@mstorsjo @hans I apologize but we will have to backport https://reviews.llvm.org/D86552 (should affect only MinGW so it's rather low risk) or revert this cherry-pick on LLVM 11 branch.

hans added a comment.Aug 26 2020, 7:48 AM

@mstorsjo @hans I apologize but we will have to backport https://reviews.llvm.org/D86552 (should affect only MinGW so it's rather low risk) or revert this cherry-pick on LLVM 11 branch.

Okay, I'll backport it once it lands (or revert if it drags out).

hans added a comment.Aug 27 2020, 5:04 AM

@mstorsjo @hans I apologize but we will have to backport https://reviews.llvm.org/D86552 (should affect only MinGW so it's rather low risk) or revert this cherry-pick on LLVM 11 branch.

Okay, I'll backport it once it lands (or revert if it drags out).

It's been cherry-picked as 522d80ab553b42e2feadfd4178932069dfc51d3f.